Skip to content

Conversation

@Tatsh
Copy link
Contributor

@Tatsh Tatsh commented Apr 29, 2025


name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo


Describe your changes
Add type hints.

Notes:

  • Improvements can certainly come after 3.13 is the oldest version of Python supported. For now a lot of things have to return Any.
  • The _KPT type used in __init__.py is less than ideal but it covers all bases of what is possible to use for a key, but there are still some functions that extend from Keyattr and those must take _K which defaults to str.
  • typing.cast() is used instead of type assertions to avoid the error from Codacy (which has the Bandit rule about assertions). Overhead is likely minimal vs assert calls but the nice thing is they do get removed in optimised mode, so the overhead can be zero in that case. I see no real issue with assertions so if you want to use those instead of cast() I can revise this. I think making a Bandit config file can quiet Codacy about the issue.

I have tried this with my youtube-unofficial project and it works as expected.

The best way to use this is with typing is to add the type on assignment:

bd = benedict({'a': {'b': 1, 'c': 2}})
b: int = bd['a.b]
c: int = bd[('a', 'c')]

In the future the way to solve this is to have a TypeBenedict to specify a schema, and then have a Mypy plugin that would resolve the actual type. This is similar to what the Mypy Django plugin does with database models.

Related issue
Issue #157

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests for the proposed changes.
  • I have run the tests and there are not errors.

@Tatsh

This comment was marked as outdated.

@Tatsh Tatsh force-pushed the typing branch 6 times, most recently from cf42df2 to ce49866 Compare April 30, 2025 16:54
@Tatsh
Copy link
Contributor Author

Tatsh commented Apr 30, 2025

Until python/typing#548 is settled, we cannot have fully generic functions
in benedict.core. And a lot of things can be solved with the new generic syntax, but it requires Python 3.13.

@Tatsh Tatsh marked this pull request as ready for review April 30, 2025 16:56
@Tatsh
Copy link
Contributor Author

Tatsh commented Apr 30, 2025

@fabiocaccamo This is ready for review. Also inviting @pirate.

Copy link

@pirate pirate left a comment

Choose a reason for hiding this comment

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

spent 10 minutes spot-checking this PR and it looks good to me, cant promise I didn't miss anything though

@codecov
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

❌ Patch coverage is 99.11504% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.68%. Comparing base (de5ea2d) to head (c307e3c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
benedict/serializers/base64.py 89.47% 2 Missing ⚠️
benedict/dicts/base/base_dict.py 98.64% 1 Missing ⚠️
benedict/serializers/cli.py 91.66% 1 Missing ⚠️
benedict/serializers/csv.py 85.71% 1 Missing ⚠️
benedict/serializers/xls.py 94.11% 1 Missing ⚠️
benedict/utils/type_util.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   97.76%   97.68%   -0.08%     
==========================================
  Files          63       63              
  Lines        2011     2246     +235     
==========================================
+ Hits         1966     2194     +228     
- Misses         45       52       +7     
Flag Coverage Δ
unittests 97.68% <99.11%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Jun 12, 2025

@Tatsh @pirate thank you for the great work you did for this PR, and sorry for the late feedback.

I added mypy to pre-commit hooks - so it gets automatically executed when running the tests.

Could you please fix the failing checks/tests? I will be glad to merge this PR once all flags will be green.

@fabiocaccamo fabiocaccamo added the enhancement New feature or request label Jun 12, 2025
@fabiocaccamo fabiocaccamo moved this to In Progress in Open Source Jun 12, 2025
@Tatsh
Copy link
Contributor Author

Tatsh commented Jun 13, 2025

@fabiocaccamo The type stubs deps have to be in pre-commit config for Mypy to work.

- Fix serializer type parameter order (JSON/Pickle: [Any,str] → [str,Any]).

- Simplify XMLSerializer to return dict[str,Any] instead of OrderedDict.

- Remove redundant type casts and unnecessary type ignores.

- Add proper generic typing to core functions (filter, invert, merge, subset).

- Improve type annotations for dict operations and return types.

- Remove unused import statements and clean up type imports.

- Add missing __future__ annotations imports where needed.

- Fix type ignore comments to be more specific and necessary only.
@fabiocaccamo fabiocaccamo merged commit 11456d4 into fabiocaccamo:main Sep 30, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Open Source Sep 30, 2025
@fabiocaccamo
Copy link
Owner

@Tatsh @pirate thank you again for this great PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants