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

Apply stricter linting #24

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Apply stricter linting #24

merged 1 commit into from
Mar 30, 2021

Conversation

paulo-ferraz-oliveira
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Mar 30, 2021

So, after the previous experience with #9 I thought we could further restrict the JavaScript code by making for stricter linting. This pull request presents that.

How it rolled:

  1. I moved from google to airbnb (https://github.com/erlef/setup-beam/actions/runs/702116828)
  2. I ran the CI command and started fixing stuff

(this actually came from a comment by Eric where he asked if return await was OK - I thought "if it isn't there's probably some linting, out there, that'll help us further")

@@ -133,15 +133,15 @@ async function getElixirVersion(exSpec0, otpVersion) {
`Requested Elixir version (from spec ${exSpec0}) not found in build listing`,
)
}
const otpMatch = otpVersion.match(/^(?:OTP-)?([^\.]+)/)
const otpMatch = otpVersion.match(/^(?:OTP-)?([^.]+)/)
Copy link
Collaborator Author

@paulo-ferraz-oliveira paulo-ferraz-oliveira Mar 30, 2021

Choose a reason for hiding this comment

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

Not a real problem, but not visible with the previous linting procedure, either.

@@ -179,8 +179,7 @@ async function getOTPVersions(osVersion) {
otpVersionsListing
.trim()
.split('\n')
.map((line) => {
debugger
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a real problem, but not visible with the previous linting procedure, either.

@@ -179,14 +179,12 @@ async function getOTPVersions(osVersion) {
otpVersionsListing
.trim()
.split('\n')
.map((line) => {
debugger
.forEach((line) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.map not really required

const otpMatch = line.match(/^(OTP-)?([^ ]+)/)

const otpVersion = otpMatch[2]
otpVersions.set(otpVersion, otpMatch[0]) // we keep the original for later reference
})
.sort()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.sort not really doing anything (apart from crashing if we use .forEach)

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I'm still getting the following messages:

"
Check licenses Expected — Waiting for status to be reported
Required
OTP 21.x / Elixir <1.9.1 Expected — Waiting for status to be reported
Required
OTP 22.0 / Elixir 1.9.1 Expected — Waiting for status to be reported
Required
OTP 22.0 / Elixir master Expected — Waiting for status to be reported
Required
OTP 23.0 / Elixir 1.11.0-rc.0 Expected — Waiting for status to be reported
Required
OTP master / Elixir 1.11.3 Expected — Waiting for status to be reported
Required
Unit tests Expected — Waiting for status to be reported
Required
"

in the above box

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I'm not sure this is what's preventing me from merging.

@starbelly
Copy link
Member

@paulo-ferraz-oliveira yes, this would prevent merging. I'd rather not disable it, but if this is going to be a consistent problem 🤔

The question is why? Why is it not running?

@paulo-ferraz-oliveira
Copy link
Collaborator Author

The question is why? Why is it not running?

These were renamed/removed, but apparently GitHub doesn't know about it 😄.

@starbelly
Copy link
Member

I see the issue...

@starbelly
Copy link
Member

starbelly commented Mar 30, 2021

@paulo-ferraz-oliveira so the issue was these outdated / no longer existent checks were still marked as required per our branch rules. Believe everything is up to date now. If you do a force push we can make sure that is so.

Edit:

@paulo-ferraz-oliveira force push not required, they are re-running now.

@starbelly
Copy link
Member

What's annoying is github adds every single test in the pre-release integration matrix as an individual status check. I have temporarily unchecked all those and will revisit after this is merged. Rationale is it's not easy to decipher some of the old status checks from the new.

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit 0b48181 into erlef:main Mar 30, 2021
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/stricter-linting branch March 30, 2021 21:02
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.

None yet

3 participants