Codecleanup #3 by GUKWAT · Pull Request #54 · ryansurf/cli-surf

GUKWAT · 2024-07-23T07:26:57Z

For better code readability and maintenance, I changed the if statements into dictionary mapping conditions

Summary by Sourcery

This pull request refactors several functions to use dictionary mappings for improved readability and maintainability. It also fixes a typo in a function name and adds unit tests for key functions.

Bug Fixes:
- Fixed typo in function name seperate_args to separate_args in helper.py and cli.py.
Enhancements:
- Refactored set_output_values function to use dictionary mapping for better readability and maintainability.
- Refactored print_ocean_data function to use dictionary mapping for displaying ocean data.
- Refactored print_forecast function to use dictionary mapping for displaying forecast data.
Tests:
- Added unit tests for print_location, set_output_values, and print_ocean_data functions in test_helper.py.

ryansurf · 2024-07-23T16:57:51Z

Hey @GUKWAT, nice job! I'll review your PR this afternoon.

In the meantime, the formatter failed... could you try to resolve those errors? You should be able to run poetry run pre-commit run --all-files in your local environment to see whats going on.

If not, I can give you a hand later. Regardless, thank you for your contribution

Also, I think you may have committed some files that shouldn't have been! I see 10 files changed in the PR, like .idea/misc.xml. Shouldn't only helper.py be altered? I can help on this too, unless I'm missing something

K-dash · 2024-07-24T00:21:15Z

+    for arg in args:
+        if arg in actions:
+            key, value = actions[arg]
+            arguments[key] = value


Great work!
IMO, there's one point I'd like to highlight. To ensure that the modifications you've made haven't caused any regressions (and that everything is functioning correctly), I recommend adding test cases to test_helper.py. This will help guarantee the integrity of the changes.

Hey @ryansurf Thank you!

I only made changes to the helper.py function and cli.py function where I fixed a typo. I am not sure how files like idea/misc.xml were altered.
I will also try to figure out why the formatter is failing but if not, I will let you know

Thank you! @K-dash

Yes I agree with you on that. I'm adding some tests to the test_helper.py file. Will push the changes for your review when I am done.

Hey @ryansurf Thank you!

I only made changes to the helper.py function and cli.py function where I fixed a typo. I am not sure how files like idea/misc.xml were altered. I will also try to figure out why the formatter is failing but if not, I will let you know

When you do git add <file>, make sure you're only adding helper.py and cli.py!

Hey @GUKWAT, hows it going? Feel free to ping me if you need any help!

Hey @ryansurf I am just working on the unit tests for helper.py. Still trying to figure out how to format the code using ruff having issues when it comes to the file path

@GUKWAT great! What are the file path errors you are dealing with?

ryansurf · 2024-07-29T18:24:07Z

@sourcery-ai review

sourcery-ai · 2024-07-29T18:24:14Z

Reviewer's Guide by Sourcery

This pull request focuses on improving code readability and maintainability by refactoring conditional logic in the src/helper.py file to use dictionary mappings. Additionally, it corrects spelling mistakes in function names and comments, and adds new test cases in tests/test_helper.py to ensure the correctness of the refactored functions.

File-Level Changes

Files	Changes
`src/helper.py` `tests/test_helper.py`	Refactored functions to use dictionary mappings for conditions and added corresponding test cases.
`src/helper.py` `src/cli.py`	Corrected spelling mistakes in function names and comments.

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 @GUKWAT - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟡 Testing: 3 issues found
🟢 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-07-29T18:25:41Z



-def print_ocean_data(arguments_dict, ocean_data_dict):
+def print_ocean_data(arguments_dict, ocean_data):


suggestion: Improve handling of None values in print_ocean_data

Instead of printing 'None' for missing values, consider using a more user-friendly message like 'Not available' or 'N/A'.

Suggested change

def print_ocean_data(arguments_dict, ocean_data):

def print_ocean_data(arguments_dict, ocean_data):

for key, value in ocean_data.items():

if value is None:

print(f"{key}: N/A")

else:

print(f"{key}: {value}")

sourcery-ai · 2024-07-29T18:25:42Z

+def test_print_location():
+    city = "Perth"
+    show_city = 1
+    captured_output = io.StringIO()
+    sys.stdout = captured_output
+    print_location(city, show_city)
+    sys.stdout = sys.__stdout__
+    expected_output = "Location: Perth"
+    assert captured_output.getvalue().strip() == expected_output.strip()


suggestion (testing): Test for print_location does not cover the case when show_city is 0

Please add a test case for when show_city is 0 to ensure that the function handles this scenario correctly.

sourcery-ai · 2024-07-29T18:25:42Z

+def test_set_output_values():
+    args = ['hw', 'show_large_wave', 'huv']
+    arguments = {}
+    expected = {"show_wave": 0, "show_large_wave": 1, "show_uv": 0}
+    assert set_output_values(args, arguments) == expected


suggestion (testing): Test for set_output_values does not cover all possible arguments

Consider adding more test cases to cover all possible arguments in the actions dictionary to ensure comprehensive testing.

Suggested change

def test_set_output_values():

args = ['hw', 'show_large_wave', 'huv']

arguments = {}

expected = {"show_wave": 0, "show_large_wave": 1, "show_uv": 0}

assert set_output_values(args, arguments) == expected

def test_set_output_values():

args = ['hw', 'show_large_wave', 'huv']

arguments = {}

expected = {"show_wave": 0, "show_large_wave": 1, "show_uv": 0}

assert set_output_values(args, arguments) == expected

args = ['show_wave', 'show_uv']

expected = {"show_wave": 1, "show_large_wave": 0, "show_uv": 1}

assert set_output_values(args, arguments) == expected

sourcery-ai · 2024-07-29T18:25:42Z

+def test_print_ocean_data():
+    arguments_dict = {
+        "show_uv": "1",
+        "show_height": "1",
+        "show_direction": "1",
+        "show_period": "0",
+        "show_air_temp": "1",
+        "show_wind_speed": "1",
+        "show_wind_direction": "0"
+    }


suggestion (testing): Test for print_ocean_data does not cover all display_mapping keys

Please add test cases to cover all keys in the display_mapping dictionary to ensure that all possible outputs are tested.

GUKWAT added 4 commits July 21, 2024 13:37

set_outout_values function refactored

3d2ac32

print_forecast function refactored

8c1851b

fixed typo function seperate_args to separate_args

b38375a

print_ocean_data method refactored

2ce2e6b

GUKWAT changed the title ~~Codecleanup~~ Codecleanup #3 Jul 23, 2024

K-dash reviewed Jul 24, 2024

View reviewed changes

K-dash mentioned this pull request Jul 24, 2024

Improve makefile and pre-commit #56

Merged

GUKWAT added 7 commits July 27, 2024 09:36

test added for print location

5105fd6

changes made to the print_location function for the unit test to pass

2f8ecc7

test for set output values created

0066ff3

print_ocean_data test created

111aab0

print ocean data function modified

bf310f6

helper file formatted

acec795

formatted helper.py

02c421a

sourcery-ai Bot reviewed Jul 29, 2024

View reviewed changes

GUKWAT added 3 commits August 2, 2024 00:37

docstrings modified for helper.py and test_helper.py

e3f6946

test created for print_location when show city is 0

8fbb0c2

print location function condition enhanced

b6bcce9



		def print_ocean_data(arguments_dict, ocean_data_dict):
		def print_ocean_data(arguments_dict, ocean_data):

-def print_ocean_data(arguments_dict, ocean_data):
+def print_ocean_data(arguments_dict, ocean_data):
+    for key, value in ocean_data.items():
+        if value is None:
+            print(f"{key}: N/A")
+        else:
+            print(f"{key}: {value}")

Conversation

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

Uh oh!

Summary by Sourcery

Uh oh!

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

Uh oh!

Uh oh!

K-dash Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GUKWAT Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

GUKWAT Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

GUKWAT Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

ryansurf commented Jul 29, 2024

Uh oh!

sourcery-ai Bot commented Jul 29, 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 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GUKWAT commented Jul 23, 2024 •

edited by sourcery-ai Bot

Loading

ryansurf commented Jul 23, 2024 •

edited

Loading

K-dash Jul 24, 2024 •

edited

Loading

sourcery-ai Bot commented Jul 29, 2024 •

edited

Loading