search

PrincetonUniversity / blocklint

MIT License
7 stars 7 forks source link

Ignore entire docstrings witha single `# blocklint ... pragma` comment #21

Closed pacrob closed 7 months ago

pacrob commented 7 months ago

Closes #20

This is a first run at allowing a single ignore comment to affect an entire single- or multi-line docstring.

I use the added clean_ignored_docstrings function to go through a list of the lines of a file and set each line to an empty string if it is within an ignored docstring. The resulting list is then passed to process_file for normal processing. I had originally considered just concatenating all the lines in a docstring and passing as a single line, but that would mess with the location information if an un-ignored blocked word was found.

troycomi commented 7 months ago

Thanks for taking the lead on this, it looks promising!

Just to be sure, the "blanking" of multi-line comments are done in such a way that line numbers are preserved?

Also, I wonder if there is a regex-based solution. It'd be complex but also more performant. My concern with the current approach is you check every block doc string and it's possible most will not have the pragma. Is it better to find all pragmas and look backwards to blank out lines?

pacrob commented 7 months ago

Just to be sure, the "blanking" of multi-line comments are done in such a way that line numbers are preserved?

It does. I created the test case first so I could verify nothing else changed.

Also, I wonder if there is a regex-based solution. It'd be complex but also more performant. My concern with the current approach is you check every block doc string and it's possible most will not have the pragma. Is it better to find all pragmas and look backwards to blank out lines?

That's a great idea! It could definitely be more performant. I'll think about it and take another stab next weekend.

pacrob commented 7 months ago

Added a new version of the clean_ignored_docstrings function. Still needs more testing.

pacrob commented 7 months ago

Ok, cleaned up and another test file added for cases I thought of. Let me know what you think!

troycomi commented 7 months ago

Looks good. Try to run precommit locally before pushing. That seems to be the only check you are failing at the moment. I'll merge and release once those pass.

Thanks again!

pacrob commented 7 months ago

Linted and pushed. Thanks!