Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ignoreInvalidInlineComments - new option to not post invalid-inline-c… #1061

Merged
merged 7 commits into from
Aug 19, 2020
Merged

ignoreInvalidInlineComments - new option to not post invalid-inline-c… #1061

merged 7 commits into from
Aug 19, 2020

Conversation

pinkasey
Copy link
Contributor

@pinkasey pinkasey commented Aug 16, 2020 •
edited
Loading

…omments in main results comment

Motivation for this feature:
My Dangerfile scans all changed files entirely*, and often finds warnings in lines that have not changed.
I want to ignore these warnings.
So I can do this in my own Dangerfile, but I thought this feature would be useful.
If you agree - I can add unit-tests.

* My dangerfile runs a static analysis tool (detekt, for kotlin), and publishes the result. that's why it often finds warnings in unchanged lines.

pinkasey
pinkasey commented Aug 17, 2020
View reviewed changes
source/runner/_tests/_executor.test.ts
new Promise<any>((_, reject) => reject())
)
platform.createInlineComment = apiFailureMock
platform.createInlineComment = jest.fn().mockReturnValue(new Promise<any>((_, reject) => reject()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really a change.
prettier reformatted these lines, and I removed the redundant apiFailureMock.

pinkasey
pinkasey commented Aug 17, 2020
View reviewed changes
source/runner/_tests/_executor.test.ts
{ message: "1", file: "1.swift", line: 1 },
{ message: "2", file: "2.swift", line: 2 },
],
warnings: [{ message: "1", file: "1.swift", line: 1 }, { message: "2", file: "2.swift", line: 2 }],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from here it's all prettier's changes

pinkasey
pinkasey commented Aug 17, 2020
View reviewed changes
source/commands/utils/sharedDangerfileArgs.ts Outdated
@@ -43,3 +45,8 @@ export default (command: any) =>
.option("-u, --passURLForDSL", "[dev] Use a custom URL to send the Danger DSL into the sub-process")
.option("-f, --failOnErrors", "Fail if there are fails in the danger report", false)
.option("--use-github-checks", "Use GitHub Checks", false)
.option(
"-iiic, --ignoreInvalidInlineComments",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiic?
WDYT?
should i just remove the short form?

@orta
Copy link
Member

orta commented Aug 17, 2020

I'm cool with this, but what about noOutOfDiffComments instead? The things being removed aren't exactly invalid, who knows what triggered it but stripping them seems reasonable to me. Can ditch the short name for that one too, ha

@pinkasey
Copy link
Contributor Author

I'm cool with this, but what about noOutOfDiffComments instead? The things being removed aren't exactly invalid, who knows what triggered it but stripping them seems reasonable to me. Can ditch the short name for that one too, ha

Awesome
I'll update the PR today

@pinkasey
Copy link
Contributor Author

@orta
a thought about noOutOfDiffComments -
I think It's not precise.
Correct me if I'm wrong -
This option ignores inline-comments that did not pass validation by github. Perhaps there are other validations that Github do, which I'm not aware of.

Anyway, if you think this is the right name - who am I to argue.
LMK if there's anything else you want me to change
(should I bump the version? I'd really like the version to be advance when this is merged, so I can pull this change to my project)

@orta
Copy link
Member

orta commented Aug 17, 2020

Good point. I agree with you, it also changes behavior, maybe we should go with ignoreOutOfDiffComments?

Nah, don't worry about the versions, I deploy manually and do that

@pinkasey
Copy link
Contributor Author

pinkasey commented Aug 18, 2020 •
edited
Loading

You mean ignoreInvalidInlineComments?
I don't see a difference between noOutOfDiffComments and ignoreOutOfDiffComments.
I prefer ignoreInvalidInlineComments, but they're all good names, no big deal.
Your decision.

@orta
Copy link
Member

orta commented Aug 19, 2020

I'd like ignoreOutOfDiffComments please - I think the ignore tells you that it won't affect the build pass/fail.

noOutOfDiffComments I would expect to still edit the build/pass but to not put the comments in the PR. I'm not sold on calling something which is outside of the PR's diff range invalid cause they could be real fails based on rules changes you have made

Thanks!

@pinkasey
Copy link
Contributor Author

Then we are decided.
ignoreOutOfDiffComments it is.

I think this PR is ready to merge...

@orta
Copy link
Member

orta commented Aug 19, 2020

rock

@orta orta merged commit 547e4a3 into danger:master Aug 19, 2020
@pinkasey pinkasey deleted the eyal_ignore_invalid_inline_comments branch August 19, 2020 10:18
@orta
Copy link
Member

orta commented Aug 19, 2020

This is shipped

@pinkasey
Copy link
Contributor Author

Just tested it - works like a charm.
Thanks!
The guys at the (home) office would be thrilled.

@roypillar
Copy link

Just tested it - works like a charm.
Thanks!
The guys at the (home) office would be thrilled.

I am a guy from the office and I can confirm that I am thrilled.

@orta
Copy link
Member

orta commented Aug 19, 2020

haha, nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pinkasey @orta @roypillar