Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Add the File::errorCodesToSuppress method, which scans a source file for all lint markers #384

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Nov 9, 2021

This dict returned by lintMarkers can be reused by all the linters, reducing repeated string search.

@Atry Atry changed the title Add lintMarkers method, which scans a source file for all lint markers Add the lintMarkers method, which scans a source file for all lint markers Nov 9, 2021
@Atry Atry marked this pull request as draft November 9, 2021 21:11
@Atry Atry marked this pull request as ready for review November 9, 2021 21:37
@Atry Atry marked this pull request as draft November 9, 2021 21:56
@Atry Atry force-pushed the marker-scaner branch 2 times, most recently from 840371a to 8ed09eb Compare November 9, 2021 21:57
@Atry Atry marked this pull request as ready for review November 9, 2021 21:58
@Atry Atry requested a review from fredemmott November 9, 2021 21:58
@Atry Atry marked this pull request as draft November 9, 2021 22:06
@Atry Atry force-pushed the marker-scaner branch 2 times, most recently from c8aed9d to 009b7b1 Compare November 9, 2021 22:40
@Atry Atry marked this pull request as ready for review November 9, 2021 23:03
@Atry Atry mentioned this pull request Nov 10, 2021
src/File.hack Outdated Show resolved Hide resolved
src/File.hack Outdated
* Returns the 1-base line number of lint markers in the file.
*/
<<__Memoize>>
public function lintMarkers(): dict<string, keyset<self::TLineNumber1Base>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

lintMarkersForLineBasedSuppression

Not all linters should respect these, especially AST-based linters (they should require that the suppression be attached to the specific AST node).

That's probably the best for hh-client based linters; given ASTs are cached, it shoudln't be too wasteful. The position can be transformed to a node via HHAST\find_node_at_position() - and the AST identifies which comments correspond to which nodes.

if memoized, this does need sharing between linters, but it's not great for separation of concerns :(

Copy link
Contributor Author

@Atry Atry Nov 10, 2021

Choose a reason for hiding this comment

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

I am not sure if HHVM supports something similar to ES6 WeakMap to implement a non-intrusive cache.

Copy link
Contributor Author

@Atry Atry Nov 10, 2021

Choose a reason for hiding this comment

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

It's not worth to introduce some mechanism of Dependency Injection in this simple library.
We might be able to implement the Cake Pattern here, but Hack traits are more restrictive than Scala traits.

Probably there is no simple solution for separation of concerns here.

src/File.hack Outdated Show resolved Hide resolved
src/File.hack Outdated Show resolved Hide resolved
@Atry Atry changed the title Add the lintMarkers method, which scans a source file for all lint markers Add the lintMarkersForLineBasedSuppression method, which scans a source file for all lint markers Nov 10, 2021
@Atry Atry force-pushed the marker-scaner branch 3 times, most recently from 9915195 to 3562ba1 Compare November 10, 2021 20:52
Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

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

Fine for these to be fixed in followups.

src/File.hack Outdated Show resolved Hide resolved
src/File.hack Outdated Show resolved Hide resolved
src/Linters/SuppressibleTrait.hack Outdated Show resolved Hide resolved
@fredemmott
Copy link
Contributor

Does need green test run though

@Atry Atry force-pushed the marker-scaner branch 3 times, most recently from 300ee11 to 31652f1 Compare November 10, 2021 21:20
… suppress according to lint markers in the file
@Atry Atry changed the title Add the lintMarkersForLineBasedSuppression method, which scans a source file for all lint markers Add the File::errorCodesToSuppress method, which scans a source file for all lint markers Nov 10, 2021
@Atry Atry merged commit 3be4574 into hhvm:main Nov 10, 2021
@Atry Atry deleted the marker-scaner branch November 10, 2021 21:31
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