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

🎨 Use constant to define default values #54

Merged
merged 1 commit into from
Aug 18, 2024
Merged

Conversation

nikhilbadyal
Copy link
Owner

@nikhilbadyal nikhilbadyal commented Aug 8, 2024

Summary by Sourcery

Refactor code to use constants for default values, improving maintainability and consistency across the project.

Enhancements:

  • Replace hardcoded default values with constants imported from a central configuration file.

@nikhilbadyal nikhilbadyal self-assigned this Aug 8, 2024
Copy link

sourcery-ai bot commented Aug 8, 2024

Reviewer's Guide by Sourcery

This pull request refactors the code to use a centralized dictionary, default_config_fields, for defining default values. This change affects the initialization of the class in esxport/click_opt/cli_options.py and the CLI options in esxport/esxport_cli.py. The default_config_fields dictionary is defined in esxport/constant.py.

File-Level Changes

Files Changes
esxport/click_opt/cli_options.py
esxport/esxport_cli.py
Refactored to use default_config_fields for default values in both the CLI options and the class initialization.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@nikhilbadyal nikhilbadyal added the chore🧹 Code Cleanup label Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (37528e8) to head (8db1577).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #54   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          306       318   +12     
=========================================
+ Hits           306       318   +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @nikhilbadyal - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • Fix incorrect attribute assignment for client_cert and client_key (link)

Overall Comments:

  • Consider further refactoring the __init__ method to reduce repetition, possibly using a dictionary comprehension or a helper function to set attributes with default values.
  • The password field is handled differently from other fields. Consider adding a comment explaining why, or handle it consistently with other fields if appropriate.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🔴 Security: 1 blocking issue
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@nikhilbadyal nikhilbadyal merged commit 0809dfa into main Aug 18, 2024
9 checks passed
@nikhilbadyal nikhilbadyal deleted the default-constant branch August 18, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore🧹 Code Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant