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

Create argument classes #109

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
206 changes: 206 additions & 0 deletions src/argument_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
from typing import ClassVar, Literal

from pydantic import BaseModel, Field


class Arguments(BaseModel):
"""
Define arguments
"""

lat: float = 0.0
long: float = 0.0
city: str = "pleasure_point"
show_wave: bool = True
show_large_wave: bool = False
show_uv: bool = True
show_height: bool = True
show_direction: bool = True
show_period: bool = True
show_city: bool = True
show_date: bool = True
show_air_temp: bool = False
show_wind_speed: bool = False
show_wind_direction: bool = False
json_output: bool = False
show_rain_sum: bool = False
show_precipitation_prob: bool = False
unit: Literal["imperial", "metric"] = "imperial"
decimal: int = Field(default=1, ge=0)
forecast_days: int = Field(default=0, ge=0, le=7)
color: str = "blue"
gpt: bool = False


class ArgumentMappings(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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 !

"""
Class for argument mappings with multiple aliases.
"""

city: str = Field(
default="pleasure_point",
description="Name of the city you want weather data from",
)
show_wave: bool = Field(
default=True,
description="Show wave information",
)
show_large_wave: bool = Field(
default=False,
description="Show large wave information",
)
show_uv: bool = Field(
default=True,
description="Show UV information",
)
show_height: bool = Field(
default=True,
description="Show wave height information",
)
show_direction: bool = Field(
default=True,
description="Show wave direction information",
)
show_period: bool = Field(
default=True,
description="Show wave period information",
)
show_city: bool = Field(
default=True,
description="Show city information",
)
show_date: bool = Field(
default=True,
description="Show date information",
)
unit: Literal["imperial", "metric"] = Field(
default="imperial",
description="Set unit system",
)
json_output: bool = Field(
default=False,
description="Enable JSON output",
)
gpt: bool = Field(
default=False,
description="Enable GPT functionality",
)
show_air_temp: bool = Field(
default=False,
description="Show air temperature",
)
show_wind_speed: bool = Field(
default=False,
description="Show wind speed",
)
show_wind_direction: bool = Field(
default=False,
description="Show wind direction",
)
show_rain_sum: bool = Field(
default=False,
description="Show rain sum",
)
show_precipitation_prob: bool = Field(
default=False,
description="Show precipitation probability",
)

alias_map: ClassVar[dict[str, list[str]]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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"],

"city": ["loc", "location"],
"show_wave": ["sw", "hide_wave", "hw"],
Copy link
Owner

Choose a reason for hiding this comment

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

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

"show_large_wave": ["slw"],
"show_uv": ["suv", "hide_uv", "huv"],
Copy link
Owner

Choose a reason for hiding this comment

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

same as show wave

"show_height": ["sh", "hide_height", "hh"],
"show_direction": ["sd", "hide_direction", "hdir"],
Copy link
Owner

Choose a reason for hiding this comment

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

same as show wave

"show_period": ["sp", "hide_period", "hp"],
"show_city": ["sc", "hide_location", "hl"],
"show_date": ["sdate", "hide_date", "hdate"],
"unit": ["m", "metric"],
"json_output": ["j", "json"],
"gpt": ["g"],
"show_air_temp": ["sat"],
"show_wind_speed": ["sws"],
"show_wind_direction": ["swd"],
"show_rain_sum": ["srs"],
"show_precipitation_prob": ["spp"],
}

@classmethod
def generate_mappings(cls) -> dict[str, str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

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]:

"""
Flatten the alias map to create a mapping from each alias to the field
name
"""
mappings = {}
for field_name, aliases in cls.alias_map.items():
for alias in aliases:
mappings[alias] = field_name
return mappings

@classmethod
def parse_input(cls, data: dict) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

alias_to_field_map = cls.generate_mappings()
remapped_data = {}
for key, value in data.items():
field_name = alias_to_field_map.get(key, key)
remapped_data[field_name] = value

return remapped_data

"""
TODO: write method to set location and update the dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""

@classmethod
def set_output_values(cls, args, arguments_dictionary: dict) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

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()}

"""
Takes a list of command line arguments(args)
and sets the appropriate values in the
arguments_dictionary(show_wave = 1, etc).
Returns the arguments_dictionary dict with the updated CLI args
"""
# map of arguments to dictionary keys & values
mappings = {
"hide_wave": ("show_wave", False),
"hw": ("show_wave", False),
"show_large_wave": ("show_large_wave", True),
"slw": ("show_large_wave", True),
"hide_uv": ("show_uv", False),
"huv": ("show_uv", False),
"hide_height": ("show_height", False),
"hh": ("show_height", False),
"hide_direction": ("show_direction", False),
"hdir": ("show_direction", False),
"hide_period": ("show_period", False),
"hp": ("show_period", False),
"hide_location": ("show_city", False),
"hl": ("show_city", False),
"hide_date": ("show_date", False),
"hdate": ("show_date", False),
"metric": ("unit", "metric"),
"m": ("unit", "metric"),
"json": ("json_output", True),
"j": ("json_output", True),
"gpt": ("gpt", True),
"g": ("gpt", True),
"show_air_temp": ("show_air_temp", True),
"sat": ("show_air_temp", True),
"show_wind_speed": ("show_wind_speed", True),
"sws": ("show_wind_speed", True),
"show_wind_direction": ("show_wind_direction", True),
"swd": ("show_wind_direction", True),
"show_rain_sum": ("show_rain_sum", True),
"srs": ("show_rain_sum", True),
"show_precipitation_prob": ("show_precipitation_prob", True),
"spp": ("show_precipitation_prob", True),
}
# Update arguments_dictionary based on the cli arguments in args
# Ex: If "hide_uv" in args,
# "show_uv" will be set to False in arguments_dictionary
for arg in args:
if arg in mappings:
key, value = mappings[arg]
arguments_dictionary[key] = value

return arguments_dictionary
Loading