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

Replace Paver quality and js_test commands #34845

Closed
7 tasks done
Tracked by #34467
kdmccormick opened this issue May 22, 2024 · 4 comments
Closed
7 tasks done
Tracked by #34467

Replace Paver quality and js_test commands #34845

kdmccormick opened this issue May 22, 2024 · 4 comments
Assignees

Comments

@kdmccormick
Copy link
Member

kdmccormick commented May 22, 2024

Context

Paver is deprecated:

We would like to fully remove it soon:

There are a small handful of Paver commands remaining in edx-platform. We need to replace these commands in order to fully remove Paver.

Tasks

Tasks

General Guidance

There is a "Commands" table on the Paver DEPR ticket. This covers all of the rows that say "Need to implement".

When choosing replacements: the best-case replacement is when we can simply run the underlying command. For example, I believe that pycodestyle will work out-of-the-box as a replacement for paver run_pep8. This is a very good developer experience, and it means that we don't need to maintain any custom tooling logic in edx-platform. It is possible that this will work for other commands, too, although some tooling configuration may need to be added in order for them to work.

The next-best-case is to have a simple Makefile target. For example, we might need to have make pii_check. If possible, try to avoid writing separate new scripts in edx-platform, as this increases complexity for end-users and adds new burden for edx-platform maintainers.

For each Paver command you replace:

  • Update the GitHub action that uses it
  • Remove the old Paver command. Paver is already deprecated, so there is no need to leave the old commands around for backwards-compatibility.
  • Leave a note on [DEPR]: Paver #34467 so that Kyle can update the table

Timeline

We would like to complete this before the Sumac cut in October so that Paver can be completely removed in the Sumac release.

@kdmccormick kdmccormick added the epic Large unit of work, consisting of multiple tasks label May 22, 2024
@feanil feanil moved this to 🆕 New in Aximprovements Team May 22, 2024
@salman2013 salman2013 self-assigned this Jun 20, 2024
@irtazaakram irtazaakram moved this from 🆕 New to 🏗 In progress in Aximprovements Team Jun 27, 2024
@jristau1984 jristau1984 removed this from Arch-BOM Jul 2, 2024
@salman2013
Copy link
Contributor

salman2013 commented Aug 5, 2024

paver run_pep8

I tried the pycodestyle . command and its working fine, there is a scenario that i face where when i run this command on the terminal it was showing errors in few files but on CI it was not showing any error, after searching and reading i found there is version difference and the version of pycodestyle on my local machine was new which has some rules changes due to which it was complaining for in few file. Overall this pycodestyle . command is working fine.

paver run_stylelint

I tried stylelint command to replace the paver, it runs but gives me 10943 errors in all .scss files which are very hard to fix. If i use this command in CI the check will always fail. As per previous implementation there is a threshold count for violations and if that threshold crosses then they fail the check otherwise the errors are just stored in an external file. My thoughts are to go with the previous implementation logic and use a violation threshold.

paver run_eslint
The same is the case with this command when i use the direct eslint command It gives a lot of errors in .js files which is hard to fix and to pass the CI check.

It's possible that the use of eslint and stylelint direct commands could cause an increase in errors if the rules have become stricter or if new rules have been added.

Image

@kdmccormick @feanil I Need your feedback on this.

@kdmccormick
Copy link
Member Author

@salman2013

pycodestyle -- great! As long as pycodestyle . passes when using the pycodestyle version pinned in testing.txt, then we are good.

stylelint and eslint -- good to know, thanks for checking that. I agree with your assessment that the existing violations would be hard to fix, so we will instead need a script to handle the threshhold. I imagine you can use a modified version of paver run_eslint and paver run_stylelint. Could you turn them into scripts under the scripts/ directory, and ensure that they don't use paver or any part of pavelib?

@salman2013 salman2013 moved this from 🏗 In progress to 👀 In review in Aximprovements Team Aug 27, 2024
@salman2013
Copy link
Contributor

PR link: #35159

@kdmccormick kdmccormick removed the epic Large unit of work, consisting of multiple tasks label Nov 14, 2024
@kdmccormick
Copy link
Member Author

Done!

@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Aximprovements Team Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants