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

Refactor test_export_html to work with modules #121

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

JoshKowi
Copy link
Collaborator

Adjusted the test_export_html to work with the New refactored modularization

Copy link
Contributor

@holybiber holybiber left a comment

Choose a reason for hiding this comment

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

Hi @JoshKowi thanks, that looks clean and good!
I noticed minor things, can you fix them?

  • Can you restore the introductory documentation? (first 12 lines, looks like you accidentally did two changes there)
  • Please delete the two lines self.config as it's not used
  • flake8 has one complaint

@JoshKowi
Copy link
Collaborator Author

Changed as requested. 👍

@JoshKowi
Copy link
Collaborator Author

About flake8: I thought that if we are going to use black anyway, it might not be important to care about flake8 - should I install black and let it run, or are we going to do that on github?

@JoshKowi JoshKowi requested a review from holybiber November 29, 2024 08:50
@holybiber holybiber merged commit 61ea19b into 4training:main Nov 29, 2024
2 checks passed
@JoshKowi JoshKowi deleted the refactor_test_export_html branch November 29, 2024 10:06
@holybiber
Copy link
Contributor

Great, thanks. Yeah that wasn't any real issue, it just looks better to have all the CI checks return green on commits and PRs...
Introducing an auto-formatter (I actually now think let's directly use ruff - #118) will happen very soon, once we have that please run it before each commit.

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

Successfully merging this pull request may close these issues.

2 participants