Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Sep 7, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ee1fc5
chore: improve `Ecosystem.md` linting script
nooreldeensalah Sep 7, 2022
02c9286
Improve ecosystem linting to include all mis-formatted in error msg
nooreldeensalah Sep 7, 2022
08d429d
Check for misformatted module patterns in all ecosystem file sections
nooreldeensalah Sep 7, 2022
70027c5
fixup! Improve ecosystem linting to include all mis-formatted in erro…
nooreldeensalah Sep 7, 2022
970e49c
revert: limit ecosystem linting to community section only
nooreldeensalah Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 13 additions & 6 deletions .github/scripts/lint-ecosystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,33 @@ module.exports = async function ({ core }) {
const moduleNameRegex = /^\- \[\`(.+)\`\]/
let hasOutOfOrderItem = false
let lineNumber = 0
let inCommmunitySection = false
let inCommunitySection = false
let modules = []

for await (const line of rl) {
lineNumber += 1
if (line.startsWith('#### [Community]')) {
inCommmunitySection = true
inCommunitySection = true
}
if (line.startsWith('#### [Community Tools]')) {
inCommmunitySection = false
inCommunitySection = false
}
if (inCommmunitySection === false) {
if (inCommunitySection === false) {
continue
}
Comment on lines +31 to 33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if (line.startsWith('- [`') !== true) {
if (line.startsWith('- [') !== true) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened another PR at #4258

continue
}

const moduleName = moduleNameRegex.exec(line)[1]
const moduleNameTest = moduleNameRegex.exec(line)

if (moduleNameTest === null)
{
return core.setFailed(`line ${lineNumber}: improper pattern, module name should be enclosed with backticks`)
}

const moduleName = moduleNameTest[1]
if (modules.length > 0) {
if (compare(moduleName, modules.at(-1)) > 0) {
core.error(`line ${lineNumber}: ${moduleName} not listed in alphabetical order`)
Expand Down