Skip to content

Conversation

@samueljlieber
Copy link
Contributor

@samueljlieber samueljlieber commented Aug 4, 2023

This PR improves the functionality of the Makefile by adding the review target as well as adding an optional rst_style check for detecting early line breaks.

The make review target enables all the optional checks to be ran, prompting the user for a path and maximum line length.

task-2801043

@samueljlieber samueljlieber self-assigned this Aug 4, 2023
@robodoo
Copy link
Collaborator

robodoo commented Aug 4, 2023

@C3POdoo C3POdoo requested a review from a team August 4, 2023 19:43
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Approving after discussion and seeing a demo.
I think this is a really great add to our test kit.

Passing to @doc-review for a more thorough look.
Thanks in advance @AntoineVDV @Feyensv 🙏

@AntoineVDV AntoineVDV force-pushed the 14.0-autolint-make-review-opt-checks-sali branch from acc14f8 to db683dc Compare August 7, 2023 11:31
@AntoineVDV AntoineVDV changed the title [IMP] Makefile: make review optional early break [IMP] test/rst_style, Makefile: add optional review checkers Aug 7, 2023
Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

Hello, I pushed a review commit that you can squash if you're okay with it.

Some parts of the logic for "early breaks" are far-fetched, IMHO, so I didn't bother rewriting them and only reformatted+refactored what I could:

  • added 100 as the default value for the line length;
  • (re-)wrote some comments to explain better what make review is about;
  • renamed a few variables and functions;
  • simplified is_valid_line;
  • fixed the yield suggesting the wrong target line.

Otherwise, it seems to work overall, even though some false positives remain.

@robodoo delegate+

@AntoineVDV AntoineVDV force-pushed the 14.0-autolint-make-review-opt-checks-sali branch from db683dc to bd76c3f Compare August 7, 2023 11:39
@samueljlieber
Copy link
Contributor Author

Hi @AntoineVDV thank you for the improvements, I made two small corrections in 455e770:

  1. the $ added at the end of the REGEX was causing many of the false positives, removing this character stopped the false positives from my testing.
  2. I believe the yield suggesting the target line number was correct:
    Screenshot 2023-08-07 at 4 11 32 PM
    Screenshot 2023-08-07 at 4 11 59 PM

I can squash and merge with your 👍 – thank you! 🙏

@AntoineVDV
Copy link
Collaborator

Hi @AntoineVDV thank you for the improvements, I made two small corrections in 455e770:

  1. the $ added at the end of the REGEX was causing many of the false positives, removing this character stopped the false positives from my testing.
  2. I believe the yield suggesting the target line number was correct:
    Screenshot 2023-08-07 at 4 11 32 PM
    Screenshot 2023-08-07 at 4 11 59 PM

I can squash and merge with your +1 – thank you! pray

Oh yes, indeed. I updated the lno after adding the $ (because it's usually good practice to accommodate the code to a regex ending with $, and not the other way around, but it's fine here) to the regex, which spans several lines, hence why it looked wrong to me.

I will squash and r+ now so we can rebase #4870 today :)

@AntoineVDV AntoineVDV force-pushed the 14.0-autolint-make-review-opt-checks-sali branch from 455e770 to 327b5c7 Compare August 8, 2023 08:23
@AntoineVDV
Copy link
Collaborator

@robodoo r+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants