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

Feature/sg 1070 Remove description property -> include it in update instead + add docstrings #206

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

Louis-Dupont
Copy link
Contributor

@Louis-Dupont Louis-Dupont commented Nov 7, 2023

Main Change

Storing the title, description, notice and warnings inside the output of each feature instead of handling it like an internal state (with properties)

Why ?

  • Simplifies the overall code. The state based approach was
    • Error prone (risk of forgetting to update the state and the description)
    • Harder to navigate through the code, and less intuitive
    • Kind of spaghetti code
    • This without any clear benefit

Other

  1. Adding more detailed docstrings for feature extractors

Why ?

  • Well this is always good to have (most classes didnt have any clear docstring)
  • Since the description property is removed, we cannot use it anymore to generate the documentation. The workaround (and probably cleaner approach anyway) is to generate the description of the feature based on the feature docstring
    • Eventually, when we will add mkdocs we won't need the documentation script anymore, but then with this PR will already have proper docstrings for each feature so it's still a win
  1. Including Classification to the auto-generated documentation

@BloodAxe
Copy link
Contributor

BloodAxe commented Nov 8, 2023

Let's change the name of PR title to a friendlier explanation (Then it is easier for github to compose release notes)

Copy link
Contributor

@BloodAxe BloodAxe left a comment

Choose a reason for hiding this comment

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

LGTM

@Louis-Dupont Louis-Dupont changed the title Feature/sg 1070 refacto description logic Feature/sg 1070 Remove description property -> include it in update instead + add docstrings Nov 8, 2023
@Louis-Dupont Louis-Dupont merged commit dcafd5a into master Nov 8, 2023
@Louis-Dupont Louis-Dupont deleted the feature/SG-1070-refacto_description_logic branch November 8, 2023 13:00
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