-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Replace Paver quality and js_test commands #35159
base: master
Are you sure you want to change the base?
Replace Paver quality and js_test commands #35159
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@feanil @kdmccormick |
@salman2013 as you remove Paver commands, please remove the tests that correspond to those commands. |
Thanks for the PR @salman2013, but this only goes part of the way. The goal of #35159 is not only to get rid of the Keep in mind that there are several things that pavelib code did that are now completely unnecessary, for example:
I recommend starting with one simple check, such as |
@kdmccormick This PR is ready for another pass. Please take a look thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work so far. Really happy to see these running without Paver.
@kdmccormick I believe this PR is ready for another pass. Please take a look thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this feedback is straightforward, except the very last item regarding subprocess.run
. I'm happy to go over that in our sync tomorrow morning if my comment isn't clear.
@kdmccormick Thanks for reviewing the PR, I have fixed all your comments, and the PR is ready for another pass. |
Taking another look... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a partial review-- I'll have some more comments tomorrow, and I still need to run each check locally. But generally this is looking good and it's exciting to see how much you were able to delete.
@@ -204,3 +204,29 @@ migrate: migrate-lms migrate-cms | |||
# Part of https://github.com/openedx/wg-developer-experience/issues/136 | |||
ubuntu-requirements: ## Install ubuntu 22.04 system packages needed for `pip install` to work on ubuntu. | |||
sudo apt install libmysqlclient-dev libxmlsec1-dev | |||
|
|||
eslint: ## check javascript for quality issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard interface from our other frontend repos is to add the linting & testing scripts to package.json, such that users execute them with npm run ...
.
Instead of adding Makefile targets for frontend scripts, could you add them to package.json? To be consistent with other repos, use:
npm run test
(for test-js)npm run coverage
(for coverage-js)npm run lint
(for eslint, stylelint, and xsslint)
coverage-js: ## run javascript coverage test | ||
python scripts/js_test.py --option coverage | ||
|
||
quality: pycodestyle eslint stylelint xsslint pii_check check_keywords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have make quality
either completely check quality (including mypy and pylint), or leave it out of the Makefile. Either is OK, up to you.
Paver is deprecated, and we aim to fully remove it soon. You can find more details here:
Paver deprecation issue
Paver removal PR
A few Paver commands remain in the edx-platform, and to proceed with its removal, we've replaced the following Paver commands with equivalent Make commands:
How to Test This PR:
In the edx-platform directory, you can use the following terminal commands to check for linting issues in Python and JavaScript files. Make sure your virtual environment is activated before running these commands.
make pycodestyle
make eslint
make stylelint
make xsslint
make pi_check
make check_keyword
make test-js
make test-coverage
make quality-test
For more details, please refer to the ticket: Replace paver quality and js_test commands