chore: improve `Ecosystem.md` linter to check for improper module name patterns #4257

nooreldeensalah · 2022-09-07T07:37:10Z

Improve ecosystem linting script by returning an error in case of an improperly added module name pattern (i.e., not enclosed with backticks)

Resolves #4256

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Improve ecosystem linting script by returning an error in case of an improperly added module name pattern (i.e., not enclosed with backticks)

mcollina

lgtm

nooreldeensalah · 2022-09-07T07:57:50Z

.github/scripts/lint-ecosystem.js

+    if (inCommunitySection === false) {
      continue
    }


I wanted to point out that the script currently only lints the modules in the #### [Community] section, it doesn't lint modules in either #### [Community Tools] or #### [Core] sections

I've re-arranged the order of the conditionals to make the script check for misformatted patterns in all sections, but it only checks for ordering in the community section (the previously existing behavior before I add any changes)

I'll keep this comment unresolved, feel free to resolve it if you don't have any comments regarding this

…r msg

jsumners

Please reduce the change set to only solve the issue being addressed. Fixing the misspelling is okay, but changing around the logic makes it difficult to review the proposed change.

nooreldeensalah · 2022-09-07T18:49:33Z

Please reduce the change set to only solve the issue being addressed. Fixing the misspelling is okay, but changing around the logic makes it difficult to review the proposed change.

The change set only addresses the issue of checking for improperly formatted module names (specifically, module names missing either or both the backticks, as in the linked issue)

I can try to make a self-review to attempt to clarify the changes in case they aren't clear, but the overall code changes can be summarized as follows:

Fixed the misspelling
Checking for improperly formatted modules in all sections (Core, Community, Community Tools)
Reduced the startsWith argument for detecting lines containing module names to '- [' instead of '- [`' (Notice the missing starting backtick), my justification is to be able to detect for all scenarios of bad backticks formatting (instead of being only able to detect the missing ending backtick only), but I haven't changed the regex pattern itself.
- Missing starting backtick
- Missing ending backtick
- Missing both starting and ending backticks
Not a change per se, but I've changed the scope of moduleName to be a global one, to be able to use the result of the regex test it in this conditional block (https://github.com/nooreldeensalah/fastify/blob/70027c57c12c64d0846fa415a7ce2e6c007472e2/.github/scripts/lint-ecosystem.js#L27-L35) that checks for the ill-formatted patterns (which constitutes most of the logic in the set of changes) and to use the result (the value of the regex test) in the conditional block checking for ordering as well.
- The continue statement (Line 34) is to avoid adding the value undefined (i.e., the result of a failed regex check due to a bad pattern) to the array of modules and order checking altogether.

Hope this comment clarifies the changes better now in case of checking the individual commits is unclear

jsumners · 2022-09-07T19:02:24Z

The issue at hand is #4256 in which want verify that items are formatted as

- [`foo`] bar

instead of allowing - [foo] bar. Please keep the PR narrowed to that specific issue instead of changing around which sections are verified and other items.

nooreldeensalah · 2022-09-07T19:11:35Z

The change set addresses this specific issue only, but I've had to make a few changes to address the issue, and I've attempted to clarify them in my comment, which code blocks in your opinion are irrelevant, so I can remove them?

I might try to make the diff as small as possible (perhaps removing the line breaks I've added and to use a local scope for moduleName but that would require doing the regex check twice 😅)

Btw, I've used https://github.com/nektos/act to test and run the script locally

jsumners · 2022-09-07T19:22:42Z

The change set addresses this specific issue only, but I've had to make a few changes to address the issue, and I've attempted to clarify them in my comment, which code blocks in your opinion are irrelevant, so I can remove them?

You stated:

I wanted to point out that the script currently only lints the modules in the #### [Community] section, it doesn't lint modules in either #### [Community Tools] or #### [Core] sections

And:

I've re-arranged the order of the conditionals to make the script check for misformatted patterns in all sections, but it only checks for ordering in the community section (the previously existing behavior before I add any changes)

This has resulted in a difficult to read change set in regards to the issue purportedly being solved, as I have outlined. Please do not change the functionality of the script in this pull request, and limit it fixing the line validation what was previously being performed.

nooreldeensalah · 2022-09-07T19:36:11Z

I apologize if this is the source of confusion, but I've made this comment when I had only the first commit 8ee1fc5, (notice that the conditional block was at the bottom).

So, I was stating the general functionality of the script, and I've stated that it currently only checks for bad module formats (but it was limited to Community section only), so I've just moved the block up (before the conditional used to check for community section to account for all the sections, but I haven't altered/removed any previously existing functionality. If this is still an issue, I can drop the remaining commits and keep the first one if you wish

nooreldeensalah · 2022-09-07T19:56:45Z

This has resulted in a difficult to read change set in regards to the issue purportedly being solved, as I have outlined. Please do not change the functionality of the script in this pull request, and limit it fixing the line validation what was previously being performed.

I've reverted back to a previous state, the script now is limited to the Community section (i.e., neither lines in Core, nor Community Tools will be validated)

Hopefully this is more clear, but if you wish that I drop the final commit (i.e., the revert commit), I can do so, but I've done this to make it the diff as small as possible, so you can easily review it.

jsumners

Looks good to me.

jsumners · 2022-09-07T21:44:39Z

.github/scripts/lint-ecosystem.js

      continue
    }

-    if (line.startsWith('- [`') !== true) {
+    if (line.startsWith('- [') !== true) {


Okay, this makes sense. We were just skipping the line if it started like - [f.

Indeed, cases like - [foo], - [foo`] were undetectable.
In regard to extending the script functionality, do you wish that I make another commit with it? (as I stated, Core and Community Tools were being validated, but I've reverted this part in my last commit)

Yes, that would be a new pull request with an improvement. I originally left those out because they are generally only added by the core Fastify team. We don't usually have out-of-order issues with those. But it doesn't hurt to add the validation.

Alright then, should I then open another PR with this state? 70027c57c1 (i.e., before the revert commit). Or are there any other changes you would like to point out to?

I'd rather just review the feature in isolation instead of trying to pick it out of other changes.

I've opened another PR at #4258

github-actions · 2023-09-08T00:19:13Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

chore: improve Ecosystem.md linting script

8ee1fc5

Improve ecosystem linting script by returning an error in case of an improperly added module name pattern (i.e., not enclosed with backticks)

mcollina approved these changes Sep 7, 2022

View reviewed changes

mcollina requested review from jsumners and Eomm September 7, 2022 07:45

nooreldeensalah commented Sep 7, 2022

View reviewed changes

nooreldeensalah added 3 commits September 7, 2022 10:51

Improve ecosystem linting to include all mis-formatted in error msg

02c9286

Check for misformatted module patterns in all ecosystem file sections

08d429d

fixup! Improve ecosystem linting to include all mis-formatted in erro…

70027c5

…r msg

nooreldeensalah changed the title ~~chore: improve Ecosystem.md linting script~~ chore: improve Ecosystem.md linter to check for improper module name patterns Sep 7, 2022

jsumners requested changes Sep 7, 2022

View reviewed changes

revert: limit ecosystem linting to community section only

970e49c

jsumners approved these changes Sep 7, 2022

View reviewed changes

jsumners merged commit af27b78 into fastify:main Sep 7, 2022

nooreldeensalah mentioned this pull request Sep 7, 2022

chore: improve Ecosystem.md linter to lint all sections #4258

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve `Ecosystem.md` linter to check for improper module name patterns #4257

chore: improve `Ecosystem.md` linter to check for improper module name patterns #4257

nooreldeensalah commented Sep 7, 2022 •

edited

mcollina left a comment

nooreldeensalah Sep 7, 2022 •

edited

nooreldeensalah Sep 7, 2022

nooreldeensalah Sep 7, 2022

jsumners left a comment

nooreldeensalah commented Sep 7, 2022 •

edited

jsumners commented Sep 7, 2022 •

edited

nooreldeensalah commented Sep 7, 2022 •

edited

jsumners commented Sep 7, 2022

nooreldeensalah commented Sep 7, 2022

nooreldeensalah commented Sep 7, 2022 •

edited

jsumners left a comment

jsumners Sep 7, 2022

nooreldeensalah Sep 7, 2022

jsumners Sep 7, 2022

nooreldeensalah Sep 7, 2022

jsumners Sep 7, 2022

nooreldeensalah Sep 7, 2022

github-actions bot commented Sep 8, 2023

chore: improve Ecosystem.md linter to check for improper module name patterns #4257

chore: improve Ecosystem.md linter to check for improper module name patterns #4257

Conversation

nooreldeensalah commented Sep 7, 2022 • edited

Checklist

mcollina left a comment

Choose a reason for hiding this comment

nooreldeensalah Sep 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

nooreldeensalah commented Sep 7, 2022 • edited

jsumners commented Sep 7, 2022 • edited

nooreldeensalah commented Sep 7, 2022 • edited

jsumners commented Sep 7, 2022

nooreldeensalah commented Sep 7, 2022

nooreldeensalah commented Sep 7, 2022 • edited

jsumners left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2023

chore: improve `Ecosystem.md` linter to check for improper module name patterns #4257

chore: improve `Ecosystem.md` linter to check for improper module name patterns #4257

nooreldeensalah commented Sep 7, 2022 •

edited

nooreldeensalah Sep 7, 2022 •

edited

nooreldeensalah commented Sep 7, 2022 •

edited

jsumners commented Sep 7, 2022 •

edited

nooreldeensalah commented Sep 7, 2022 •

edited

nooreldeensalah commented Sep 7, 2022 •

edited