refactor: simplify and consolidate helper.py and cli.py#183
Merged
ryansurf merged 2 commits intoFeb 27, 2026
Conversation
- Extract shared _extract_arg() helper; simplify extract_decimal, get_forecast_days, and get_color to thin wrappers - Merge surf_summary into print_gpt (was only called there) - Simplify round_decimal to a list comprehension - Use enumerate in forecast_to_json instead of range(len(...)) - Fix typo: seperate_args -> separate_args (alias kept for compat) - cli.py: unpack set_location tuple directly - cli.py: compute json_output once before if/else branch
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Reviewer's GuideRefactors shared CLI/helper logic by introducing a generic argument extractor, simplifying rounding and forecast JSON construction, merging the GPT surf summary into a single function, and cleaning up cli.run to unpack locations and reuse a single JSON computation, all while preserving behavior and adding a backwards-compatible alias for the old argument-splitting helper. Sequence diagram for updated cli.run execution and JSON handlingsequenceDiagram
actor User
participant CLI as cli_run
participant Helper as helper
participant API as api
participant DB as db_handler
User->>CLI: run(lat?, long?, args?)
alt args is None
CLI->>Helper: separate_args(sys.argv)
Helper-->>CLI: args
else args provided
CLI->>Helper: separate_args(args)
Helper-->>CLI: args
end
CLI->>API: seperate_args_and_get_location(args)
API-->>CLI: location
CLI->>Helper: set_location(location)
Helper-->>CLI: city, loc_lat, loc_long
alt lat == 0 and long == 0
CLI->>CLI: lat, long = loc_lat, loc_long
end
CLI->>Helper: arguments_dictionary(lat, long, city, args)
Helper-->>CLI: arguments
CLI->>API: gather_data(lat, long, arguments)
API-->>CLI: ocean_data_dict
CLI->>Helper: json_output(ocean_data_dict, print_output=False)
Helper-->>CLI: json_out
alt arguments[json_output] == 0
CLI->>Helper: print_outputs(ocean_data_dict, arguments, gpt_prompt, gpt_info)
Helper-->>CLI: response
alt db_uri is set
CLI->>DB: insert_report(json_out)
end
CLI-->>User: ocean_data_dict, response
else JSON output requested
alt db_uri is set
CLI->>DB: insert_report(json_out)
end
CLI-->>User: json_out
end
Class diagram for updated helper.py argument and GPT utilitiesclassDiagram
class HelperModule {
+arguments_dictionary(lat, long, city, args)
+set_output_values(args, arguments_dictionary)
+separate_args(args)
+seperate_args(args)
+_extract_arg(args, keys, default, cast)
+extract_decimal(args)
+get_forecast_days(args)
+get_color(args)
+print_location(city, show_city)
+print_ocean_data(arguments_dict, ocean_data_dict)
+print_forecast(ocean, forecast)
+round_decimal(round_list, decimal)
+json_output(data_dict, print_output)
+print_outputs(ocean_data_dict, arguments, gpt_prompt, gpt_info)
+set_location(location)
+forecast_to_json(forecast_data, decimal)
+print_gpt(surf_data, gpt_prompt, gpt_info)
}
class ArgExtractionFlow {
+arguments_dictionary(lat, long, city, args)
+extract_decimal(args)
+get_forecast_days(args)
+get_color(args)
+_extract_arg(args, keys, default, cast)
}
class GPTFlow {
+print_outputs(ocean_data_dict, arguments, gpt_prompt, gpt_info)
+print_gpt(surf_data, gpt_prompt, gpt_info)
}
HelperModule <.. ArgExtractionFlow
HelperModule <.. GPTFlow
ArgExtractionFlow --> HelperModule : uses
GPTFlow --> HelperModule : uses
ArgExtractionFlow ..> _extract_arg : internal_shared_helper
HelperModule ..> _extract_arg : internal_use
HelperModule ..> extract_decimal : wraps__extract_arg
HelperModule ..> get_forecast_days : wraps__extract_arg
HelperModule ..> get_color : wraps__extract_arg
GPTFlow ..> print_gpt : builds_summary_and_selects_model
HelperModule ..> print_outputs : orchestrates_printing_and_forecast
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/helper.py" line_range="165-174" />
<code_context>
+ return value
+
+
+def get_forecast_days(args):
+ """
+ Extract forecast day count from CLI args. Defaults to 0. Max is 7.
+ """
+ MAX_VALUE = 7
+ value = _extract_arg(args, ["forecast", "fc"], default=0, cast=int)
+ if value < 0 or value > MAX_VALUE:
+ print("Must choose a non-negative number <= 7 in forecast!")
+ return 0
</code_context>
<issue_to_address>
**suggestion:** Consider making the forecast-days error handling and message use the configured default and max consistently.
Invalid values currently log a message and reset to `0`, even though you already define `MAX_VALUE` and pass a default of `0` to `_extract_arg`. It would be clearer to interpolate `MAX_VALUE` in the message (e.g. `<= {MAX_VALUE}`) and return the same default value used by `_extract_arg` instead of a hardcoded `0`, so behavior stays consistent if the default or max change later.
```suggestion
def get_forecast_days(args):
"""
Extract forecast day count from CLI args. Defaults to 0. Max is 7.
"""
DEFAULT_VALUE = 0
MAX_VALUE = 7
value = _extract_arg(args, ["forecast", "fc"], default=DEFAULT_VALUE, cast=int)
if value < 0 or value > MAX_VALUE:
print(
f"Must choose a non-negative number <= {MAX_VALUE} in forecast! "
f"Using default ({DEFAULT_VALUE})."
)
return DEFAULT_VALUE
return value
```
</issue_to_address>
### Comment 2
<location path="src/helper.py" line_range="375-377" />
<code_context>
+ f"swell is {surf_data['Swell Direction']} degrees and the swell "
+ f"period is {surf_data['Period']} seconds."
+ )
+ api_key, gpt_model = gpt_info
+ MIN_KEY_LEN = 5
+ if not api_key or len(api_key) < MIN_KEY_LEN:
+ return gpt.simple_gpt(summary, gpt_prompt)
+ return gpt.openai_gpt(summary, gpt_prompt, api_key, gpt_model)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** API key validation in `print_gpt` may treat whitespace-only keys as valid.
The condition `if not api_key or len(api_key) < MIN_KEY_LEN` considers whitespace-only strings (length ≥ 5) as valid and sends them to `openai_gpt`, likely causing a less controlled failure. Consider normalizing first, e.g. `normalized = (api_key or '').strip()`, then checking `if not normalized or len(normalized) < MIN_KEY_LEN:` and passing `normalized` onward.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+165
to
+174
| def get_forecast_days(args): | ||
| """ | ||
| Extract forecast day count from CLI args. Defaults to 0. Max is 7. | ||
| """ | ||
| MAX_VALUE = 7 | ||
| value = _extract_arg(args, ["forecast", "fc"], default=0, cast=int) | ||
| if value < 0 or value > MAX_VALUE: | ||
| print("Must choose a non-negative number <= 7 in forecast!") | ||
| return 0 | ||
| return value |
Contributor
There was a problem hiding this comment.
suggestion: Consider making the forecast-days error handling and message use the configured default and max consistently.
Invalid values currently log a message and reset to 0, even though you already define MAX_VALUE and pass a default of 0 to _extract_arg. It would be clearer to interpolate MAX_VALUE in the message (e.g. <= {MAX_VALUE}) and return the same default value used by _extract_arg instead of a hardcoded 0, so behavior stays consistent if the default or max change later.
Suggested change
| def get_forecast_days(args): | |
| """ | |
| Extract forecast day count from CLI args. Defaults to 0. Max is 7. | |
| """ | |
| MAX_VALUE = 7 | |
| value = _extract_arg(args, ["forecast", "fc"], default=0, cast=int) | |
| if value < 0 or value > MAX_VALUE: | |
| print("Must choose a non-negative number <= 7 in forecast!") | |
| return 0 | |
| return value | |
| def get_forecast_days(args): | |
| """ | |
| Extract forecast day count from CLI args. Defaults to 0. Max is 7. | |
| """ | |
| DEFAULT_VALUE = 0 | |
| MAX_VALUE = 7 | |
| value = _extract_arg(args, ["forecast", "fc"], default=DEFAULT_VALUE, cast=int) | |
| if value < 0 or value > MAX_VALUE: | |
| print( | |
| f"Must choose a non-negative number <= {MAX_VALUE} in forecast! " | |
| f"Using default ({DEFAULT_VALUE})." | |
| ) | |
| return DEFAULT_VALUE | |
| return value |
Comment on lines
+375
to
+377
| api_key, gpt_model = gpt_info | ||
| MIN_KEY_LEN = 5 | ||
| if not api_key or len(api_key) < MIN_KEY_LEN: |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): API key validation in print_gpt may treat whitespace-only keys as valid.
The condition if not api_key or len(api_key) < MIN_KEY_LEN considers whitespace-only strings (length ≥ 5) as valid and sends them to openai_gpt, likely causing a less controlled failure. Consider normalizing first, e.g. normalized = (api_key or '').strip(), then checking if not normalized or len(normalized) < MIN_KEY_LEN: and passing normalized onward.
Owner
|
Hey @raza-khan0108, thanks for the contribution! The code definitely looks cleaner with these changes. It lgtm, but can you make sure you run the linter and check on the failing test? |
Contributor
Author
|
Sure, i will take a look at it and make the necessary changes |
- Wrap long lines in helper.py and cli.py to stay within 79 char limit - Use ruff auto-format on both files to ensure formatter passes - Restore extract_decimal's exact error message to fix test_invalid_input: 'Invalid value for decimal. Please provide an integer.'
Contributor
Author
Changes Made
|
Owner
|
@raza-khan0108 this looks great, thanks for the contribution |
Owner
|
@all-contributors please add @raza-khan0108 for refactor |
Contributor
|
I couldn't determine any contributions to add, did you specify any contributions? |
Owner
|
@all-contributors please add @raza-khan0108 for refactoring code |
Contributor
|
I've put up a pull request to add @raza-khan0108! 🎉 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor: simplify and consolidate
helper.pyandcli.pyCleans up duplicated and verbose patterns across both files, reducing overall line count by ~158 lines with no behaviour changes.
helper.py_extract_arg()private helper —extract_decimal,get_forecast_days, andget_colorall shared the same "scan args forkey=value, parse, return default" loop; replaced with thin wrappers over the shared helpersurf_summaryintoprint_gpt— it had no callers outside ofprint_gptround_decimalto a list comprehensionrange(len(...))withenumerateinforecast_to_jsonseperate_args→separate_args; old name kept as an alias for backwards compatibilitycli.pyset_locationreturn value directly instead of manually indexing[0],[1],[2]json_outputonce before theif/elsebranch instead of duplicating the call in both branchesGeneral:
Code:
Summary by Sourcery
Refactor helper and CLI argument handling to reduce duplication and simplify control flow without changing observable behaviour.
Enhancements: