Create argument classes #109

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

src/argument_types.py

+        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

src/argument_types.py

+        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

src/argument_types.py

+    """
+
+    @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

src/argument_types.py

+    }
+
+    @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

src/argument_types.py

+        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

src/argument_types.py

+
+    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

src/argument_types.py

+        "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

src/argument_types.py

+        "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

src/argument_types.py

+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create argument classes #109

Create argument classes #109

avkoll commented Aug 9, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Aug 9, 2024 •

edited

Loading

sourcery-ai bot left a comment

sourcery-ai bot Aug 9, 2024

sourcery-ai bot Aug 9, 2024

sourcery-ai bot Aug 9, 2024

sourcery-ai bot Aug 9, 2024

sourcery-ai bot Aug 9, 2024

ryansurf commented Aug 12, 2024 •

edited

Loading

ryansurf Aug 12, 2024

ryansurf Aug 12, 2024

ryansurf Aug 12, 2024

K-dash commented Aug 13, 2024

K-dash Aug 13, 2024

ryansurf Aug 15, 2024

-    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

Create argument classes #109

Are you sure you want to change the base?

Create argument classes #109

Conversation

avkoll commented Aug 9, 2024 • edited by sourcery-ai bot Loading

Example Usage:

Summary by Sourcery

sourcery-ai bot commented Aug 9, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Aug 9, 2024

Choose a reason for hiding this comment

sourcery-ai bot Aug 9, 2024

Choose a reason for hiding this comment

sourcery-ai bot Aug 9, 2024

Choose a reason for hiding this comment

sourcery-ai bot Aug 9, 2024

Choose a reason for hiding this comment

sourcery-ai bot Aug 9, 2024

Choose a reason for hiding this comment

ryansurf commented Aug 12, 2024 • edited Loading

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

ryansurf Aug 12, 2024

Choose a reason for hiding this comment

K-dash commented Aug 13, 2024

K-dash Aug 13, 2024

Choose a reason for hiding this comment

ryansurf Aug 15, 2024

Choose a reason for hiding this comment

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