Skip to content

fix: update and add quality tests#3

Merged
samuelallan72 merged 4 commits intomainfrom
samuel/quality
Sep 29, 2025
Merged

fix: update and add quality tests#3
samuelallan72 merged 4 commits intomainfrom
samuel/quality

Conversation

@samuelallan72
Copy link
Copy Markdown
Member

@samuelallan72 samuelallan72 commented Sep 23, 2025

Description

  • update requirements
  • update pylint from edx-lint
  • configure quality tests and linters: pylint, isort, black
  • fix the code to pass the lints
  • update .github files

Supporting information

Private-ref: https://tasks.opencraft.com/browse/BB-10035

Test instructions

Deadline

None

@samuelallan72 samuelallan72 self-assigned this Sep 23, 2025
@samuelallan72 samuelallan72 marked this pull request as ready for review September 23, 2025 01:37
@samuelallan72 samuelallan72 marked this pull request as draft September 23, 2025 01:38
@samuelallan72 samuelallan72 changed the title wip: update and add tests fix: update and add quality tests Sep 23, 2025
@samuelallan72 samuelallan72 marked this pull request as ready for review September 23, 2025 06:08
@samuelallan72 samuelallan72 force-pushed the samuel/quality branch 8 times, most recently from cf6ff9c to 5da3951 Compare September 23, 2025 06:24
Comment thread pylintrc_tweaks
@Kelketek
Copy link
Copy Markdown
Member

@samuelallan72 I've done a readthrough and these changes make sense to me. I may circle back to test the endpoints manually after some other work today, but I'm just as happy to let @tecoholic test it since I think he's already got it set up.

@samuelallan72
Copy link
Copy Markdown
Member Author

@tecoholic the diff is fairly large, and I was worried about creating subtle bugs by mixing refactoring with reformatting. So I split this into 3 commits, which may make it easier to review separately:

  1. changes to all the scaffolding files
  2. reformatting (this is only the result of running make format)
  3. refactoring and tweaks to fix quality checks

A few minor things changed in the process (nothing functional) - you can check those with the "compare" button next to the force push message above.

cc @Kelketek

Comment thread pylintrc
Copy link
Copy Markdown
Member

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@samuelallan72 Amazing work on getting this repo to shape. If we sort out Black's config to match Pylint's rules on line-legth, I think we are good here.

Comment thread pylintrc
Comment thread setup.cfg
@@ -1,8 +1,5 @@
[isort]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Related to the comment on Black & line legth - we can replace this setup.cfg with pyproject.toml.

Comment thread setup.py
Comment thread setup.py
"License :: Other/Proprietary License",
"Natural Language :: English",
"Programming Language :: Python :: 3",
"Programming Language :: Python :: 3.5",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"Programming Language :: Python :: 3.5",
"Programming Language :: Python :: 3.11",

Comment thread setup.py
Comment thread shoppingcart/migrations/0005_new_initial.py
Comment thread shoppingcart/models.py Outdated
@tecoholic
Copy link
Copy Markdown
Member

@samuelallan72 Oh! Thank you for the 3 clean commits. It was a breeze to review the PR.

Random passwords and enrolment tokens are generated here.
The `random` module is a pseudo random number generator,
and not suitable for generating secure secrets.
Instead we can use the `secrets` module as a drop in replacement here.
@samuelallan72
Copy link
Copy Markdown
Member Author

Thanks @tecoholic ! :)

I addressed your comment about the line length, and responded to your other comments. Not sure if you'd like any of your other comments addressed here before merge?

I also added a commit to fix a minor security issue - 6a60578

I did force push sorry, just to keep the commits all neat still. I guess next time I should submit them as separate PRs to avoid that.

This should be ready for another pass. :)

Copy link
Copy Markdown
Member

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@samuelallan72 👍 LGTM

  • I tested this: Ran the quality checks and formatting locally.
  • I read through the code
  • I checked for accessibility issues - NA
  • Includes documentation

@samuelallan72 samuelallan72 merged commit 1a9bd97 into main Sep 29, 2025
1 check passed
@samuelallan72 samuelallan72 deleted the samuel/quality branch September 29, 2025 01:33
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.

3 participants