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

Prevent unexpected token error on old node versions #292

Merged
merged 5 commits into from
Jan 25, 2021

Conversation

joe-sharp
Copy link
Contributor

@joe-sharp joe-sharp commented Jan 24, 2021

I found this issue on a workstation I didn't realize had such an old version of node. If there is no interest in supporting old versions I am fine with abandoning this change. Only node v8.16.0 failed without this change (out of the versions I tested.) The following versions do not require this change; v10.16.1, v11.15.0, or v14.13.1

Proposed Changes

  1. Add exception parameter to catch

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
    Updated CHANGELOG

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

- On node v8.16.0 mega-linter-runner fails to run
- According to MDN it can be omitted if unused but
  it doesn't appear to hurt anything on many versions
  of node
@joe-sharp
Copy link
Contributor Author

Not sure I understand the failure here. If you want to proceed with this PR can you please provide some direction on resolving the issue? Not sure if it is related but I did get an error trying to run bash build.sh before pushing.

@nvuillam
Copy link
Member

What was the error when you run bash buid.sh ?

About the CI error, it's my fault, i'm on it ^^

@nvuillam
Copy link
Member

@joe-sharp

  • i added a pre-requisites section in the documentation, processing it should make bash build.sh run properly :)

  • I updated CI management from forks, please can you make your branch up to date with master branch and try again ? ( tip: write quick build in the latest commit message to run it faster )

  • Please can you also update CHANGELOG ?

Thanks for your feedback :)

@joe-sharp
Copy link
Contributor Author

What was the error when you run bash buid.sh ?

Sorry about not already providing that. I was already away from the computer when I commented. Here is the error I am seeing:

  File "./.automation/build.py", line 47
    f"{REPO_HOME}/megalinter/descriptors/schemas/megalinter-descriptor.jsonschema.json"
                                                                                      ^
SyntaxError: invalid syntax

I reran the commands from the prerequisite section but I am still seeing the error.

@joe-sharp
Copy link
Contributor Author

It looks like the issue may persist but I did have to push that subsequent merge commit separately from the quick build commit. I failed to merge my fork's master with yours before pushing. 🤦🏼

@nvuillam
Copy link
Member

I think that you have an old python version, who does not support f-strings

Please can you try to install latest Python3 and try again ? :)

@nvuillam
Copy link
Member

and about CI, there is still a bug on my side, sorry ^^

@joe-sharp
Copy link
Contributor Author

I think that you have an old python version, who does not support f-strings

Please can you try to install latest Python3 and try again ? :)

Will do, thanks! That seems like a likely issue. 😄

@nvuillam
Copy link
Member

@joe-sharp I fixed the CI and tested it from a secondary account with a fork, it should be ok if now you merge nvuillam/mega-linter@master in your branch :)

@joe-sharp
Copy link
Contributor Author

@joe-sharp I fixed the CI and tested it from a secondary account with a fork, it should be ok if now you merge nvuillam/mega-linter@master in your branch :)

Thanks much, building now...

@joe-sharp
Copy link
Contributor Author

I think that you have an old python version, who does not support f-strings

Please can you try to install latest Python3 and try again ? :)

~/Documents/repos/mega-linter 🔮❯❯❯ python3 -V
Python 3.9.1
~/Documents/repos/mega-linter 🔮❯❯❯ bash build.sh
  File "./.automation/build.py", line 47
    f"{REPO_HOME}/megalinter/descriptors/schemas/megalinter-descriptor.jsonschema.json"
                                                                                      ^
SyntaxError: invalid syntax

@nvuillam
Copy link
Member

Merged, thanks for your contribution :)
I'll release it tonight or tomorrow in an official version but can already use it in 50mn ( or in 10 if you use a Mega-Linter flavor) with insiders version :)

@nvuillam nvuillam merged commit 9295dbb into oxsecurity:master Jan 25, 2021
@nvuillam
Copy link
Member

@joe-sharp
hmmm as it is in the npm package, you can already try npx mega-linter-runner@beta .... :)

@joe-sharp
Copy link
Contributor Author

Works perfectly!

~/Documents/repos/mega-linter 🔮❯❯❯ nvm use node 8.16.0
Now using node v8.16.0 (npm v6.14.5)
~/Documents/repos/mega-linter 🔮❯❯❯ npx mega-linter-runner -v
Unexpected token {
~/Documents/repos/mega-linter 🔮❯❯❯ npx mega-linter-runner@beta -v
mega-linter-runner version 4.26.1-beta202101252205.0
~/Documents/repos/mega-linter 🔮❯❯❯ nvm use node 14.13.1
Now using node v14.13.1 (npm v6.14.8)
~/Documents/repos/mega-linter 🔮❯❯❯ npx mega-linter-runner@beta -v
mega-linter-runner version 4.26.1-beta202101252205.0

@joe-sharp joe-sharp deleted the fix_unexpected_token branch January 25, 2021 22:14
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

2 participants