Improved docs #335
Open
Improved docs #335
Conversation
This file contains 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
Reviewer's Guide by SourceryThis pull request adds a comprehensive suite of example tests to the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #335 +/- ##
===========================================
- Coverage 70.03% 59.53% -10.51%
===========================================
Files 13 8 -5
Lines 1325 939 -386
Branches 114 99 -15
===========================================
- Hits 928 559 -369
+ Misses 372 361 -11
+ Partials 25 19 -6 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- These examples are great, but consider adding a README or tutorial to guide users on how to run them.
- It might be helpful to include a section on error handling and edge cases within the examples.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| filtered_packager = Packager(options) | ||
|
|
||
| # Download the filtered data | ||
| filtered_packager.download() |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract duplicate code into function (
extract-duplicate-method) - Simplify sequence length comparison (
simplify-len-comparison)
|
|
||
| try: | ||
| # Create a table for the UNIHAN data | ||
| cursor.execute(""" |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
| ) | ||
|
|
||
| # Verify we created some educational data | ||
| assert len(educational_data) > 0 |
There was a problem hiding this comment.
suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)
Suggested change
| assert len(educational_data) > 0 | |
| assert educational_data |
| pinyin_to_chars[pinyin_key].append(item["char"]) | ||
|
|
||
| # Verify our input method dictionary has entries | ||
| assert len(pinyin_to_chars) > 0 |
There was a problem hiding this comment.
suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)
Suggested change
| assert len(pinyin_to_chars) > 0 | |
| assert pinyin_to_chars |
| ) | ||
|
|
||
| # Verify we found some correspondences | ||
| assert len(sound_correspondences) > 0 |
There was a problem hiding this comment.
suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)
Suggested change
| assert len(sound_correspondences) > 0 | |
| assert sound_correspondences |
|
|
||
| # Verify we extracted data | ||
| if data is not None: | ||
| assert len(variants_data) > 0 |
There was a problem hiding this comment.
suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)
Suggested change
| assert len(variants_data) > 0 | |
| assert variants_data |
| f"for {item.get('char', 'Unknown')}" | ||
| ) | ||
|
|
||
| char = item.get("char", "") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Simplify sequence length comparison (
simplify-len-comparison)
Comment on lines
+43
to
+48
| fields_per_file = {} | ||
| for filename, fields in UNIHAN_MANIFEST.items(): | ||
| fields_per_file[filename] = len(fields) | ||
|
|
||
| # Verify we have field counts | ||
| assert len(fields_per_file) > 0 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Convert for loop into dictionary comprehension (
dict-comprehension) - Simplify sequence length comparison (
simplify-len-comparison)
tony
force-pushed
the
improved-docs
branch
2 times, most recently
from
1ee19cc to
34763ba
Compare
… examples This commit resolves all test failures in the example test suite by: 1. Adding proper type annotations across all example tests: - Use modern Python type hints (e.g., `list[dict[str, Any]]` instead of `List[Dict[str, Any]]`) - Add proper type casts (`cast()`) for handling ambiguous return types - Fix incorrect type signatures for function parameters and return values - Ensure consistent type annotation style across all test files 2. Fixing test implementation issues: - Replace invalid field 'kFrequency' with supported fields in educational_tools test - Change 'kRSKangXi' to 'kRSUnicode' in stroke_order test - Simplify stroke_order test to use existing data rather than creating a new packager - Fix handling of list-type values by properly converting them to strings - Add proper null-checks and defensive programming for external data 3. Improve code quality: - Replace if-else blocks with ternary operators for conciseness - Convert for-loops to list comprehensions where appropriate - Add proper error handling for data conversion operations - Fix line length issues to comply with style guidelines - Add meaningful debug output for troubleshooting 4. Ensure test robustness: - Add fallback mechanisms for tests that depend on specific data patterns - Improve assertions to verify data integrity - Add type guards to prevent runtime errors with ambiguous types All tests now pass consistently, type checking with mypy succeeds with zero issues, and code formatting conforms to project standards. These changes improve code maintainability, readability, and reliability while providing example code that demonstrates best practices for using the unihan-etl library.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
Improved Docs
Summary by Sourcery
Adds example code demonstrating various use cases of the
unihan-etllibrary, including linguistic analysis, educational tools, data integration, software development, research analysis, input method development, stroke order extraction, and API development.Tests:
unihan-etllibrary for different applications.