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.

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

chore: Lint eco system error #4275

Merged
merged 8 commits into from Sep 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
106 changes: 88 additions & 18 deletions .github/scripts/lint-ecosystem.js
Expand Up @@ -4,28 +4,41 @@ const path = require('path')
const fs = require('fs')
const readline = require('readline')

const ecosystemDocFile = path.join(__dirname, '..', '..', 'docs', 'Guides', 'Ecosystem.md')
const basePathEcosystemDocFile = path.join('docs', 'Guides', 'Ecosystem.md')
const ecosystemDocFile = path.join(__dirname, '..', '..', basePathEcosystemDocFile)
const failureTypes = {
improperFormat: 'improperFormat',
outOfOrderItem: 'outOfOrderItem'
}

module.exports = async function ({ core }) {
const results = await runCheck()
await handleResults({ core }, results)
}

async function runCheck () {
const stream = await fs.createReadStream(ecosystemDocFile)
const rl = readline.createInterface({
input: stream,
crlfDelay: Infinity
});
})

const failures = []
const successes = []
const moduleNameRegex = /^\- \[\`(.+)\`\]/
let hasOutOfOrderItem = false
let lineNumber = 0
let modules = []
let hasImproperFormat = false
let grouping = 'core'

for await (const line of rl) {
lineNumber += 1
if (line.startsWith('#### [Community]')) {
grouping = 'community'
modules = []
}

if (line.startsWith('#### [Community Tools]')) {
grouping = 'community-tools'
modules = []
}

Expand All @@ -34,37 +47,94 @@ module.exports = async function ({ core }) {
}

const moduleNameTest = moduleNameRegex.exec(line)

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

if (moduleNameTest === null) {
failures.push({
lineNumber,
grouping,
moduleName: 'unknown',
type: failureTypes.improperFormat
})
continue
}

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`)
hasOutOfOrderItem = true
failures.push({
lineNumber,
moduleName,
grouping,
type: failureTypes.outOfOrderItem
})
} else {
successes.push({ moduleName, lineNumber, grouping })
}
} else {
// We have to push the first item found or we are missing items from the list
successes.push({ moduleName, lineNumber, grouping })
}
modules.push(moduleName)
}

if (hasOutOfOrderItem === true) {
core.setFailed('Some ecosystem modules are not in alphabetical order.')
}
return { failures, successes }
}

async function handleResults (scriptLibs, results) {
const { core } = scriptLibs
const { failures, successes } = results;
const isError = !!failures.length;

await core.summary
.addHeading(isError ? `❌ Ecosystem.md Lint (${failures.length} error${failures.length === 1 ? '' : 's' })` : '✅ Ecosystem Lint (no errors found)')
.addTable([
[
{ data: 'Status', header: true },
{ data: 'Section', header: true },
{ data: 'Module', header: true },
{ data: 'Details', header: true }],
...failures.map((failure) => [
'❌',
failure.grouping,
failure.moduleName,
`Line Number: ${failure.lineNumber.toString()} - ${failure.type}`
]),
...successes.map((success) => [
'✅',
success.grouping,
success.moduleName,
'-'
])
])
.write()

if (isError) {
failures.forEach((failure) => {
if (failure.type === failureTypes.improperFormat) {
core.error('The module name should be enclosed with backticks', {
title: 'Improper format',
file: basePathEcosystemDocFile,
startLine: failure.lineNumber
})
} else if (failure.type === failureTypes.outOfOrderItem) {
core.error(`${failure.moduleName} not listed in alphabetical order`, {
title: 'Out of Order',
file: basePathEcosystemDocFile,
startLine: failure.lineNumber
})
} else {
core.error('Unknown error')
}
})

if (hasImproperFormat === true) {
core.setFailed('Some ecosystem modules are improperly formatted.')
core.setFailed('Failed when linting Ecosystem.md')
}
}

function compare(current, previous) {
function compare (current, previous) {
return previous.localeCompare(
current,
'en',
{sensitivity: 'base'}
{ sensitivity: 'base' }
)
}
8 changes: 1 addition & 7 deletions .github/workflows/lint-ecosystem-order.yml
@@ -1,12 +1,6 @@
name: Lint Ecosystem Order

on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

Whi this has been changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't clear on the intent, as the previous version should run on every branch push not named main / master which would mean it will run outside of a PR and 2x on a PR (push + pull_request). In the current version it only runs on PRs (which I believe is the desired state).

In addition, the script will only work on PRs. If we want to support PRs and not PRs, I need to tweak the script to run differently depending on the context & we should probably have 2 workflows.

I also wouldn't understand if its not running on main/master why we'd ever believe its not running on a PR....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsumners hey what was intent behind push and pull_request trigger?

Copy link
Member

Choose a reason for hiding this comment

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

I think in past, che pull_request event was not triggered when new commits where pushed to the PR's branch.

Thanks for the clarification

I think it is good with this lighter setup

cc/ @Fdawgs

branches-ignore:
- master
- main
paths:
- "**/Ecosystem.md"
pull_request:
branches:
- master
Expand All @@ -28,7 +22,7 @@ jobs:
- name: Lint Doc
uses: actions/github-script@v6
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const script = require('./.github/scripts/lint-ecosystem.js')
await script({ core })