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/113/implement proper module registration #119

Conversation

GabrielSousa02
Copy link
Collaborator

What?

#113 Refactor CLI to include option for LanguagePostProcessors, instead of keeping the hardcoded options only.

Why?

This will facilitate for the user to select what should be executed by the script.

How?

Refactoring the ABC class by adding a constructor and then specializing the constructor per each class. Updating the CLI argparser to include the modules as options, creating default value for the modules (all) and then revamping the iteration over all selected modules to use one single initializer, since now we have the same constructor signatures, and then calling the run() method.

Testing?

Only two tests were failing, need to review them under, test_resourcesbot.py:

  • test_rewrite_options
  • test_run_with_cache

Screenshots (optional)

Anything Else?

- create the modules' field
- add to docstring
- add logic to call specified modules
- test_consistency_checks.py
- test_write_lists.py
- test_write_report.py
- test_write_sidebar_messages.py
- test_write_summary.py
- Change constructors for `LanguagePostProcessor`s
- Change the `Resourcesbot` constructor
- Update the `AVAILABLE_MODULES` const
- Update iteration of the post-processors, instantiating the class and calling `run()`
@holybiber holybiber merged commit 3892adc into 4training:main Nov 22, 2024
1 of 2 checks passed
@holybiber
Copy link
Contributor

Finally done!

  • I realized that logically force_rewrite actually should go into the run() method, not into the constructor -> changed that
  • I implemented dynamic loading with the load_module() function. Python has some nice functionality with @classmethod so that I can directly extract all relevant information for the command line options from the classes itself
  • no need for that workaround with putting the rewrite option into the config

Feedback for next time:

  • there were two modules which weren't committed with git mv so it was harder to see the changes
  • some files were reformatted with black already but that polluted the diffs

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.

2 participants