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.

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

Suppress lint errors file by file #418

Merged
merged 7 commits into from
Dec 9, 2021
Merged

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Dec 9, 2021

No description provided.

@Atry Atry changed the title Suppress 5639 lint errors pre-file instead of for the whole src/Migrations directory Suppress lint errors pre-file Dec 9, 2021
@Atry Atry changed the title Suppress lint errors pre-file Suppress lint errors pre file Dec 9, 2021
@Atry Atry changed the title Suppress lint errors pre file Suppress lint errors file by file Dec 9, 2021
@Atry Atry requested a review from fredemmott December 9, 2021 01:38
*/
namespace Facebook\HHAST;
use namespace Facebook\TypeAssert;
use namespace HH\Lib\Dict;
/* HHAST_IGNORE_ALL[5607] 5607 is ignored because of false positives when comparing a generic to a typed value */
/* HHAST_IGNORE_ALL[5624] HHAST_IGNORE_ALL[5639] 5624 and 5639 are ignored because they insist on using co(tra)variant generics. Could this break external consumers? */
Copy link
Contributor

@fredemmott fredemmott Dec 9, 2021

Choose a reason for hiding this comment

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

This linter (edit: 5639) might also be good for the default blacklist - there's an ongoing discussion about removing it entirely at https://fb.workplace.com/groups/hackforhiphop/posts/7429940763721141

In short: It's accurate, but it's highly opinionated, and there's not consensus on the behavior it encourages being a good thing.

This isn't the same as the other codes we'd want to always-ignore, but "pretend it doesn't exist" may be the right thing to do for "may be deleted soon". Perhaps making it off-by-default but explicitly-includable may be better.

}
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

Copy link
Contributor

Choose a reason for hiding this comment

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

... for the whole file changes, not specifically the bracket :)

Copy link
Contributor Author

@Atry Atry Dec 9, 2021

Choose a reason for hiding this comment

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

Seems like I did something wrong when resolving conflicts

This is not a real problem, just because the diff tool does not match the structural changes unfortunately.

@Atry Atry merged commit a3b3f2b into hhvm:main Dec 9, 2021
@Atry Atry deleted the suppress-5639-per-file branch December 9, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants