Create argument classes by avkoll · Pull Request #109 · ryansurf/cli-surf

avkoll · 2024-08-09T14:21:45Z

I created the argument_types.py class file similar to what @K-dash had suggested but didn't want to start integrating it into the cli.py yet since that was a huge task. I was testing it by checking and verifying the output of the following code in run() in cli.py.

Example Usage:

default_arguments = Arguments()
print(json.dumps(default_arguments.model_dump(), indent=4))

arguments = Arguments(**parsed_arguments)
# parse inputs
parsed_input = ArgumentMappings.parse_input({"lat": lat, "long": long})
updated_arguments = default_arguments.model_copy(update=parsed_input)

# update arguments with inputs
arguments_dict = updated_arguments.model_dump()
updated_arguments_dict = ArgumentMappings.set_output_values(args, arguments_dict)
arguments = updated_arguments.model_copy(update=updated_arguments_dict)

print("Updated arguments: ")
print(json.dumps(updated_arguments.model_dump(), indent=4))

for key, value in updated_arguments:
    if value is False:
        print("false")
    else:
        print(key)

I can write a test for this but wanted to open the PR incase anyone had any suggestions, if it looks good I can start writing the tests.

I wanted someone to check this before I started replacing stuff.

Summary by Sourcery

Add new classes Arguments and ArgumentMappings to manage and map command-line arguments with default values and multiple aliases. These classes include methods for parsing input and setting output values based on command-line arguments.

New Features:

Introduce Arguments class to define and manage various command-line arguments with default values.
Introduce ArgumentMappings class to handle multiple aliases for command-line arguments and provide methods for parsing input and setting output values.

sourcery-ai · 2024-08-09T14:21:52Z

Reviewer's Guide by Sourcery

This pull request introduces a new file argument_types.py that defines two classes, Arguments and ArgumentMappings, to handle command-line arguments and their mappings. The Arguments class defines various arguments with default values, while the ArgumentMappings class provides methods to parse input data and update argument dictionaries based on command-line arguments. The changes are isolated to the new file and do not yet integrate with the existing cli.py file.

File-Level Changes

Files	Changes
`src/argument_types.py`	Introduced new argument handling classes (`Arguments` and `ArgumentMappings`) with methods for parsing input and setting output values based on command-line arguments.

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.

sourcery-ai

Hey @avkoll - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider simplifying the argument handling structure by merging ArgumentMappings functionality into the Arguments class, utilizing Pydantic's built-in alias feature to reduce duplication and improve maintainability.
Add comprehensive docstrings and comments to explain the purpose and functionality of each class and method, particularly for complex operations like set_output_values.
Proceed with writing tests for this new argument handling system to ensure its correctness and facilitate future maintenance and refactoring.

Here's what I looked at during the review

🟡 General issues: 5 issues found
🟢 Security: all looks good
🟢 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.}

sourcery-ai · 2024-08-09T14:23:26Z

+        description="Show precipitation probability",
+    )
+
+    alias_map: ClassVar[dict[str, list[str]]] = {


suggestion: Consider revising the alias mapping for clarity and consistency

The current alias mapping might lead to confusion, especially with aliases like 'hide_wave' for 'show_wave'. Consider using a more consistent naming scheme or restructuring this to make the relationship between aliases and their corresponding actions more explicit.

alias_map: ClassVar[dict[str, list[str]]] = { "city": ["loc", "location"], "show_wave": ["sw"], "hide_wave": ["hw"],

sourcery-ai · 2024-08-09T14:23:26Z

+        return remapped_data
+
+    """
+    TODO: write method to set location and update the dictionary


issue: Remove or implement the TODO comment

TODO comments should not be left in production code. Either implement the method to set location and update the dictionary, or remove this comment if it's no longer relevant.

sourcery-ai · 2024-08-09T14:23:27Z

+    """
+
+    @classmethod
+    def set_output_values(cls, args, arguments_dictionary: dict) -> dict:


suggestion: Refactor to use alias_map and reduce code duplication

The set_output_values method duplicates some of the logic from alias_map. Consider refactoring this method to use alias_map directly, which would reduce duplication and potential inconsistencies in the future.

Suggested change

def set_output_values(cls, args, arguments_dictionary: dict) -> dict:

@classmethod

def set_output_values(cls, args, arguments_dictionary: dict) -> dict:

return {cls.alias_map.get(arg, arg): value for arg, value in arguments_dictionary.items()}

sourcery-ai · 2024-08-09T14:23:27Z

+    }
+
+    @classmethod
+    def generate_mappings(cls) -> dict[str, str]:


suggestion (performance): Consider caching the result of generate_mappings

If generate_mappings is called frequently, it might be more efficient to compute the result once and store it, rather than creating a new dictionary on each call.

@classmethod @functools.lru_cache(maxsize=None) def generate_mappings(cls) -> dict[str, str]:

sourcery-ai · 2024-08-09T14:23:27Z

+        return mappings
+
+    @classmethod
+    def parse_input(cls, data: dict) -> dict:


suggestion: Handle potential key conflicts in parse_input

The parse_input method doesn't handle the case where a key might be both a valid field name and an alias. Consider adding a check for this scenario to ensure consistent behavior.

Suggested change

def parse_input(cls, data: dict) -> dict:

@classmethod

def parse_input(cls, data: dict) -> dict:

alias_to_field_map = cls.generate_mappings()

field_to_alias_map = {v: k for k, v in alias_to_field_map.items()}

remapped_data = {}

for key, value in data.items():

if key in alias_to_field_map:

remapped_data[alias_to_field_map[key]] = value

elif key in field_to_alias_map:

remapped_data[key] = value

ryansurf · 2024-08-12T18:59:10Z

@avkoll

Nice work! Looks good to me, go ahead and start the integration. I'm able to help if you run into any blocks

I left a brief review, I was confused on a couple lines

ryansurf · 2024-08-12T19:00:07Z

+
+    alias_map: ClassVar[dict[str, list[str]]] = {
+        "city": ["loc", "location"],
+        "show_wave": ["sw", "hide_wave", "hw"],


Why is show wave mapped to both "sw" (which i assume means "show wave") and "hide_wave"?

ryansurf · 2024-08-12T19:00:23Z

+        "city": ["loc", "location"],
+        "show_wave": ["sw", "hide_wave", "hw"],
+        "show_large_wave": ["slw"],
+        "show_uv": ["suv", "hide_uv", "huv"],


same as show wave

ryansurf · 2024-08-12T19:00:31Z

+        "show_large_wave": ["slw"],
+        "show_uv": ["suv", "hide_uv", "huv"],
+        "show_height": ["sh", "hide_height", "hh"],
+        "show_direction": ["sd", "hide_direction", "hdir"],


same as show wave

K-dash · 2024-08-13T07:48:56Z

@avkoll
Thanks for your time!
Will review tonight.

K-dash · 2024-08-13T13:08:26Z

+    gpt: bool = False
+
+
+class ArgumentMappings(BaseModel):


I think the implementation is as I expected, so it looks good!

If I may add a suggestion.
The fields in the ArgumentMappings class overlap with those in the Arguments class. Ideally, if possible, I think it would be better to consolidate all arguments into the Arguments class and create the final mapping dictionary based on the contents of the Arguments class.(For example, when implementing new arguments in the future, we would likely need to add fields to both classes, resulting in dual management, which could lead to reduced maintainability.)

However, I believe that realizing this would require modifications to the cli.py file as well, which could potentially be a substantial amount of work. Therefore, I'll leave the decision on whether to implement this up to you :)
It's perfectly fine to keep this as a future refactoring phase if you prefer!

@ryansurf How do you think?

Agreed, it would be ideal to reduce any redundancy.

cli.py would indeed need to be revamped. Let us know what you want to do @avkoll !

avkoll added 13 commits August 7, 2024 18:15

create argument_types.py and began populating with parameters

d3fadf9

add map for arguments

3fbc3b8

add methods to generate the mappings

b26433a

move set_output_values to argument_types class

2438757

began using argument_types into main

6fcff87

fixed imports and populate_by_name parameter

3d3a7a3

class init works and methods work, comments at bottom are for testing

39c5c39

added mappings for location

3bb5054

removed comments at bottom of argument_types.py

51cf1a7

removed unused import

a10e222

began converting old cli.py to use new classes

5d990c6

remove class use in cli.py

3e8cac7

ran pre commit tests

849e16d

sourcery-ai Bot reviewed Aug 9, 2024

View reviewed changes

ryansurf reviewed Aug 12, 2024

View reviewed changes

K-dash reviewed Aug 13, 2024

View reviewed changes

-    def parse_input(cls, data: dict) -> dict:
+    @classmethod
+    def parse_input(cls, data: dict) -> dict:
+        alias_to_field_map = cls.generate_mappings()
+        field_to_alias_map = {v: k for k, v in alias_to_field_map.items()}
+        remapped_data = {}
+        for key, value in data.items():
+            if key in alias_to_field_map:
+                remapped_data[alias_to_field_map[key]] = value
+            elif key in field_to_alias_map:
+                remapped_data[key] = value

Conversation

avkoll commented Aug 9, 2024 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example Usage:

Summary by Sourcery

Uh oh!

sourcery-ai Bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

File-Level Changes

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

K-dash commented Aug 13, 2024

Uh oh!

K-dash Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

avkoll commented Aug 9, 2024 •

edited by sourcery-ai Bot

Loading

sourcery-ai Bot commented Aug 9, 2024 •

edited

Loading

ryansurf commented Aug 12, 2024 •

edited

Loading