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

Refactoring Python code to eliminate code smells #492

Merged
merged 4 commits into from Feb 12, 2024

Conversation

ekiwaka
Copy link
Contributor

@ekiwaka ekiwaka commented Feb 12, 2024

Fixed a few code smells as suggested by Sonar Lint

Changes proposed in this PR include:
"Remove this commented out code."
-- Link: https://sonarcloud.io/project/issues?open=AYcsbV4WMh9La1tIBkbn&id=PowerGridModel_power-grid-model
-- This issue was fixed in src/power_grid_model/core/power_grid_meta.py
-- Code that was commented out was removed from the function.

"Define a constant instead of duplicating this literal " and " 3 times."
-- Link: https://sonarcloud.io/project/issues?open=AYcsbV5BMh9La1tIBkbq&id=PowerGridModel_power-grid-model
-- This issue was fixed in src/power_grid_model/validation/errors.py
-- A variable named delimiter is added as a class attribute which can then be used by class functions.

"Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed."
-- Link: https://sonarcloud.io/project/issues?open=AYmXiBpXOdT128GMx9_u&id=PowerGridModel_power-grid-model
-- This issue was fixed in src/power_grid_model/validation/validation.py
-- Instead of multiple if functions, a dictionary and a for loop were used to iterate through the error data. This way, you're looping through the dictionary component_validators, checking if each component exists in the data dictionary, and if it does, you apply the corresponding validation function to that component's data. This approach makes the code more concise and easier to maintain.

I, ekiwaka <karan.rawat13@gmail.com>, hereby add my Signed-off-by to this commit: 68e5879

Signed-off-by: ekiwaka <karan.rawat13@gmail.com>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

Hi @ekiwaka ,

First of all, thank you for your contribution! It excites us that more people are starting to join us. You're actually the first external contributor that makes a code change. Are you ok with us announcing that at the next PGM meet-up?

The proposed changes are looking good. Good design choice on the validators! I only have one small nit-pick remark but it is good to go.

src/power_grid_model/validation/errors.py Outdated Show resolved Hide resolved
@Jerry-Jinfeng-Guo
Copy link
Contributor

@ekiwaka If you could rename the PR to a better descriptive title, I'll further go over the changes and give the approving review once you have processed comments from Marthijn.

@ekiwaka ekiwaka changed the title Resolving code smells from Sonar Cloud in Python side, part 1 Refactoring Python code to eliminate code smells Feb 12, 2024
@ekiwaka
Copy link
Contributor Author

ekiwaka commented Feb 12, 2024

@Jerry-Jinfeng-Guo I have made the changes requested by @mgovers , and I have renamed the PR. Let me know if this is better.

@mgovers
Copy link
Member

mgovers commented Feb 12, 2024

@ekiwaka the changes look good. Can you fix the DCO issue and run the black formatting? You can do it in the same commit

I recommend just doing 2 separate commits with the DCO message, where the second one reverses the first one.

If you really know what you're doing you can also amend the current comment

I edited the initial recommendation because the formatting failed

I, ekiwaka <karan.rawat13@gmail.com>, hereby add my Signed-off-by to this commit: 3c8446f

Signed-off-by: ekiwaka <karan.rawat13@gmail.com>
@ekiwaka
Copy link
Contributor Author

ekiwaka commented Feb 12, 2024

@mgovers done both

@mgovers
Copy link
Member

mgovers commented Feb 12, 2024

Thank you @ekiwaka . LGTM

@mgovers mgovers merged commit 4353d99 into PowerGridModel:main Feb 12, 2024
26 checks passed
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.

None yet

3 participants