KEMBAR78
Suppress lint errors file by file by Atry · Pull Request #418 · hhvm/hhast · GitHub
Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Dec 9, 2021

No description provided.

@Atry Atry force-pushed the suppress-5639-per-file branch from b881ce8 to 56f4c38 Compare December 9, 2021 01:36
@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
@Atry Atry force-pushed the suppress-5639-per-file branch from 56f4c38 to 1acb8ca Compare December 9, 2021 17:00
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.

"ignore": [5607, 5624, 5639]
}
}
]
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants