Fix corrupted wave art on Windows and clean up codebase#202
Open
SparshGarg999 wants to merge 5 commits into
Open
Fix corrupted wave art on Windows and clean up codebase#202SparshGarg999 wants to merge 5 commits into
SparshGarg999 wants to merge 5 commits into
Conversation
- Added a fallback ASCII art for the large wave if running on a Windows system without a UTF-8 encoding, preventing braille character corruption. - Refactored `src/cli.py` to use a list comprehension for cleaner mapping. - Refactored `src/helper.py` to consolidate mapping iterations with a shared internal function `_print_mapped_data`. - Confirmed that tide data (from NOAA API) accurately specifies tide direction and meets issue requirements without extra additions. Co-authored-by: SparshGarg999 <76697238+SparshGarg999@users.noreply.github.com>
…673391208369 Fix corrupted wave art on Windows and clean up codebase
Contributor
Reviewer's GuideRefactors CLI flag handling and helper printing logic for better readability/reuse, and fixes corrupted large-wave art on Windows by adding an ASCII fallback when UTF-8 braille characters are not supported. Sequence diagram for Windows wave art fallback selectionsequenceDiagram
participant Caller
participant print_wave
participant sys
participant stdout
Caller->>print_wave: print_wave(show_wave, show_large_wave, color)
alt show_large_wave
print_wave->>sys: access platform
print_wave->>stdout: access encoding
alt [sys.platform == win32 and stdout.encoding.lower() != utf-8]
print_wave->>stdout: print(ASCII_wave_art)
else [UTF-8 supported]
print_wave->>stdout: print(braille_wave_art)
end
else show_wave
print_wave->>stdout: print(small_wave_art)
end
Flow diagram for shared mapped data printing helperflowchart TD
print_ocean_data[print_ocean_data] --> _print_mapped_data[_print_mapped_data]
print_forecast[print_forecast] --> _print_mapped_data[_print_mapped_data]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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, and left some high level feedback:
- In
art.print_wave,sys.stdout.encodingcan beNonein some environments, so calling.lower()on it may raise anAttributeError; consider guarding with a default (e.g.,encoding = (sys.stdout.encoding or "").lower()or similar) before comparing to"utf-8". - The Windows encoding check in
art.print_wavecurrently only treats an exact"utf-8"as Unicode-safe, which will misclassify encodings like"cp65001"that can handle the braille characters; consider using a more robust check (e.g.,'utf' in encodingorcodecs.lookup(encoding).name == "utf-8") to avoid unnecessarily falling back to the ASCII art.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `art.print_wave`, `sys.stdout.encoding` can be `None` in some environments, so calling `.lower()` on it may raise an `AttributeError`; consider guarding with a default (e.g., `encoding = (sys.stdout.encoding or "").lower()` or similar) before comparing to `"utf-8"`.
- The Windows encoding check in `art.print_wave` currently only treats an exact `"utf-8"` as Unicode-safe, which will misclassify encodings like `"cp65001"` that can handle the braille characters; consider using a more robust check (e.g., `'utf' in encoding` or `codecs.lookup(encoding).name == "utf-8"`) to avoid unnecessarily falling back to the ASCII art.
## Individual Comments
### Comment 1
<location path="src/art.py" line_range="75" />
<code_context>
- print(
- colors[color]
- + """
+ if sys.platform == "win32" and sys.stdout.encoding.lower() != "utf-8":
+ print(
+ colors[color]
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `sys.stdout.encoding` being `None` before calling `.lower()`.
In some environments (e.g., when output is redirected), `sys.stdout.encoding` can be `None`, so `.lower()` would raise `AttributeError`. Consider something like:
```python
encoding = getattr(sys.stdout, "encoding", None) or ""
if sys.platform == "win32" and encoding.lower() != "utf-8":
...
```
This keeps the logic but avoids the potential runtime error.
</issue_to_address>
### Comment 2
<location path="src/helper.py" line_range="206-212" />
<code_context>
print("\n")
+def _print_mapped_data(mappings, arguments_dict, data_dict):
+ """
+ Helper function to print mapped data from a dictionary
+ if the argument is set.
+ """
+ for arg_key, data_key, label in mappings:
+ if arguments_dict.get(arg_key) and data_key in data_dict:
+ print(f"{label}{data_dict[data_key]}")
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `.get()` instead of direct indexing for `arguments_dict` changes behavior and may hide missing keys.
Previously, missing keys raised `KeyError`, making configuration problems explicit; now they’re silently ignored and the corresponding output is skipped. If all `mappings` keys are expected to be present in `ocean`, this relaxes validation and can hide misconfigurations. To preserve strictness, either use `arguments_dict[arg_key]` here (letting `KeyError` surface) or validate that all required keys exist before calling `_print_mapped_data`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Guard against `sys.stdout.encoding` being `None` before checking "utf-8" safely with `codecs.lookup()`. - Use strict dictionary access `arguments_dict[arg_key]` rather than `.get(arg_key)` in `_print_mapped_data()` to avoid masking missing configuration keys. Co-authored-by: SparshGarg999 <76697238+SparshGarg999@users.noreply.github.com>
…okup-15692117913474018793 Fix `sys.stdout.encoding` fallback and enforce strict dict lookup
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
1 participant
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.
General:
[x] Have you followed the guidelines in our Contributing document?
Code:
Explanation of changes:
src/cli.pyto use a list comprehension for cleaner argument flag mapping. Refactoredsrc/helper.pyto consolidate mapping iterations with a shared internal function_print_mapped_data.Summary by Sourcery
Fix Windows-specific corruption of large wave ASCII art and simplify flag and data-mapping logic across the CLI and helpers.
Bug Fixes:
Enhancements: