Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List wildcard #114

Closed

Conversation

milhauzindahauz
Copy link

implements #113

type_util.py - add wildcard index, add only dicts check
__init__.py - clean naive test run
keylist_dict.py - add condition for wildcard key
keylist_util.py - enhance condition for wildcard
tests - modify test
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Base: 96.96% // Head: 96.59% // Decreases project coverage by -0.36% ⚠️

Coverage data is based on head (71d23c0) compared to base (e7a5bfd).
Patch coverage: 92.53% of modified lines in pull request are covered.

❗ Current head 71d23c0 differs from pull request most recent head b928196. Consider uploading reports for the commit b928196 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   96.96%   96.59%   -0.37%     
==========================================
  Files          57       57              
  Lines        1678     1793     +115     
==========================================
+ Hits         1627     1732     +105     
- Misses         51       61      +10     
Flag Coverage Δ
unittests 96.59% <92.53%> (-0.37%) ⬇️

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

Impacted Files Coverage Δ
benedict/__init__.py 100.00% <ø> (ø)
benedict/dicts/__init__.py 100.00% <ø> (ø)
benedict/dicts/keylist/keylist_util.py 94.91% <90.90%> (-5.09%) ⬇️
benedict/utils/type_util.py 98.30% <92.30%> (-1.70%) ⬇️
benedict/dicts/keylist/keylist_dict.py 97.39% <93.33%> (-2.61%) ⬇️
benedict/dicts/keypath/keypath_util.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@fabiocaccamo fabiocaccamo left a comment

Choose a reason for hiding this comment

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

Thank you very much the good PR and the effort.

This PR brings wildcard * support to lists when using keypaths, with this feature all methods that accept a key should works as correctly and as expected when a * is present, but there are not tests at all for all other methods except for the rename method.

As this is an important feature that involves the whole library it needs to be tested well against all methods, a couple of new test cases are mandatory:

  • tests.dicts.test_keypath_dict_list_wildcard (core operations)
  • tests.dicts.test_benedict_wildcard (methods implementation)

benedict/__init__.py Outdated Show resolved Hide resolved
benedict/utils/type_util.py Outdated Show resolved Hide resolved
benedict/utils/type_util.py Outdated Show resolved Hide resolved
benedict/dicts/keylist/keylist_dict.py Outdated Show resolved Hide resolved
benedict/dicts/keylist/keylist_dict.py Outdated Show resolved Hide resolved
benedict/dicts/keylist/keylist_util.py Show resolved Hide resolved
tests/dicts/test_benedict.py Outdated Show resolved Hide resolved
tests/dicts/test_benedict.py Outdated Show resolved Hide resolved
benedict/dicts/keypath/keypath_util.py Show resolved Hide resolved
benedict/dicts/keypath/keypath_util.py Outdated Show resolved Hide resolved
remove unnecessary imports
simplify condition
rename function, rename variable
use util function
change conditions blocks
remove print function from test
Copy link
Owner

@fabiocaccamo fabiocaccamo left a comment

Choose a reason for hiding this comment

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

Could you provide also the test cases?

@milhauzindahauz
Copy link
Author

milhauzindahauz commented Sep 13, 2022 via email

add wildcard regex, change _split_key_indexes() to use it
Copy link
Owner

@fabiocaccamo fabiocaccamo left a comment

Choose a reason for hiding this comment

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

We should be ok with the changes, just need the tests.

I'm pretty sure that adding the tests against all methods there will be something to adjust.

@milhauzindahauz
Copy link
Author

I am going through methods of benedict class. I am trying to figure out all the methods for tests.

fixes TypeError benedict methods (e.g. swap())
build few tests, move wildcard test to separate test file
benedict/utils/type_util.py Outdated Show resolved Hide resolved
benedict/dicts/keylist/keylist_dict.py Outdated Show resolved Hide resolved
tests/dicts/test_benedict_wildcard.py Outdated Show resolved Hide resolved
tests/dicts/test_benedict.py Outdated Show resolved Hide resolved
add is_list to is_list_of_dicts
change tests
@milhauzindahauz
Copy link
Author

milhauzindahauz commented Sep 14, 2022 via email

fix test
add condition for wildcard in keys
@milhauzindahauz
Copy link
Author

@fabiocaccamo,

if you want to cover this more thoroughly by test, can you help me and compile the list of methods you want to cover?
Thanks

@fabiocaccamo
Copy link
Owner

@milhauzindahauz I would absolutely cover all methods with at least a couple of different cases (eg. wildcard alone / at the beginning / in the middle / at the end of keypaths), this library is widely used, I don't want to see tens of issues open after this feature will be released.

pre-commit-ci bot and others added 2 commits October 18, 2022 13:49
change pop behavior for wildcard. Change test to match it.
@milhauzindahauz
Copy link
Author

Apologies for the pre-commit errors but there is configured max-line-length for flake8 in setup.cfg. But it seems to me that black is using the default value in the CI. Am I wrong?

fix missed patch
enhance test
remove dead code
add test for uncovered code
simplify code, remove unnecessary logic for wildcard index
@fabiocaccamo
Copy link
Owner

Apologies for the pre-commit errors but there is configured max-line-length for flake8 in setup.cfg. But it seems to me that black is using the default value in the CI. Am I wrong?

Thank you for noticing it, probably there is a conflict between Black, .flake8 config file and setup.cfg.

make test more complex, simplify condition to fix bug
@milhauzindahauz
Copy link
Author

Apologies for the pre-commit errors but there is configured max-line-length for flake8 in setup.cfg. But it seems to me that black is using the default value in the CI. Am I wrong?

Thank you for noticing it, probably there is a conflict between Black, .flake8 config file and setup.cfg.

black doesn't support setup.cfg. I configured manually according to setup.cfg. It's my mistake. But I suggest to change it to blacks default value 88 in setup.cfg to avoid the mismatch.

milhauzindahauz and others added 8 commits October 30, 2022 09:47
use generators for wildcard on upper levels
introduce low_level_generator
remove dead code
refactor generators
add test for pop call on complex data structure
use generators for wildcard on upper levels
remove unused code
reduce complexity
remove death code
remove unused variable
Copy link
Author

@milhauzindahauz milhauzindahauz left a comment

Choose a reason for hiding this comment

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

I don't have anything else to this feature.

elif __item:
yield __item
elif type_util.is_dict(_item) and index in _item:
yield _item[index]
Copy link
Owner

Choose a reason for hiding this comment

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

If _item is tuple, it will not enter any of the conditions above.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the purpose of this method, could you explain it please?

Copy link
Author

Choose a reason for hiding this comment

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

this method is allowing to travers through complex list and return references to objects on the lowest level of them.

Copy link
Owner

@fabiocaccamo fabiocaccamo left a comment

Choose a reason for hiding this comment

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

The PR doesn't pass tests.

@milhauzindahauz
Copy link
Author

The PR doesn't pass tests.

I will need to make changes, again.

@fabiocaccamo fabiocaccamo deleted the branch fabiocaccamo:master January 18, 2023 09:32
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.

2 participants