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

test: do not lint macros files #24885

Closed
wants to merge 1 commit into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Dec 7, 2018

These four .py files do not contain any Python code. Instead they contain "macros" which are processed by js2c.py. Our Python linter (make lint-py) will properly flag these files as containing Python syntax errors. One solution would be to change the filename suffixes (.py -->.js2c) or we could add a linter directive (# noqa) to the comments of each file to tell our linter to ignore those files. This PR implements the latter.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Dec 7, 2018
@targos
Copy link
Member

targos commented Dec 7, 2018

The change in deps/v8/src/js/macros.py should be done upstream

@targos
Copy link
Member

targos commented Dec 7, 2018

or alternatively, we shouldn't lint our deps

@cclauss
Copy link
Contributor Author

cclauss commented Dec 7, 2018

You are correct. I forgot about #24512 (If you can help on that it would be much appreciated.)

We do not lint our deps but some of these files are not in ./deps.

I will change this PR to revert the current changes and instead add these files to the excludes of make lint-py.

@cclauss
Copy link
Contributor Author

cclauss commented Dec 7, 2018

Closing... make lint-py already does the right thing.

@cclauss cclauss closed this Dec 7, 2018
@cclauss cclauss deleted the Python-macros branch December 7, 2018 07:28
@cclauss cclauss restored the Python-macros branch December 7, 2018 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants