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

Add Reverse lookup #43

Merged
merged 14 commits into from
Dec 13, 2023
Merged

Conversation

vincentdavis
Copy link
Contributor

Issue #42, reverse lookup
I have implemented this as a class that initiates from the UUID dicts. I did it this way rather than writing the index to a file for two reasons; 1: it's fast enough, 2: In the future, we might want to add the ability to rebuild the index incases where the user adds custom UUIDs.
Feedback appreciated.
I am not very familiar with taking full advantage of pytest.

@vincentdavis
Copy link
Contributor Author

Just made a few updates to fix doc test and test annotations

@koenvervloesem
Copy link
Owner

Do you have pre-commit installed? It catches the same errors as found by the CI here, but locally.

See https://bluetooth-numbers.readthedocs.io/en/stable/contributing.html#clone-the-repository for the installation.

@vincentdavis
Copy link
Contributor Author

Do you have pre-commit installed? It catches the same errors as found by the CI here, but locally.

See https://bluetooth-numbers.readthedocs.io/en/stable/contributing.html#clone-the-repository for the installation.

I am working on that right now.

@vincentdavis
Copy link
Contributor Author

vincentdavis commented Dec 10, 2023

@koenvervloesem Kinda struggling with the pre-commit. I have a lot of it cleaned up.
I don't think I have everything installed to fully run the pre-commit and my editor, Pycharm, is not using ruff configuration correctly. It's not giving me the hints and warnings or fixing the import sorting as I expect.

I don't seem to have all the type hints correct, but again not getting clear information from Pycharm as to whats wrong.

I am kinda stuck and out of time for now.

Maybe a development requirements file would be helpful?

@koenvervloesem
Copy link
Owner

Thanks already for the work you did! I'll take a look at the pre-commit errors tomorrow and see what I can fix. Maybe we can also ignore some of the errors if they're too pedantic.

@koenvervloesem
Copy link
Owner

I added some comments, these should solve/ignore most of the errors. But have a look at the doctests too.

@vincentdavis
Copy link
Contributor Author

I added some comments, these should solve/ignore most of the errors. But have a look at the doctests too.

Ok, I have cleaned up most of this, I'll finish up later today.

@koenvervloesem
Copy link
Owner

koenvervloesem commented Dec 12, 2023

Ok, just one issue with the import order (see above), and the two doctest issues I reviewed, and then this looks ready to merge.

@koenvervloesem koenvervloesem mentioned this pull request Dec 12, 2023
@vincentdavis
Copy link
Contributor Author

I think it's ready.
The interrogate pre-commit does not work for me locally. I like the idea of what it does. Maybe using Ruff's pydocstyle would be simpler and sufficient.
https://docs.astral.sh/ruff/rules/#pydocstyle-d

@koenvervloesem
Copy link
Owner

If you click on Details next to the failing tests, there are still some issues. Could you take a look at them? Once these tests run successfully, we can merge this PR.

@vincentdavis
Copy link
Contributor Author

vincentdavis commented Dec 12, 2023

@koenvervloesem
looking at the failing doc test, not sure what I am doing wrong. Is it sensitive to whitespace (2 lines vs 1 line?

Failed example:
    rl.lookup("Power Feature", uuid_types=['characteristic'],logic="SUBSTR")
Expected:
    {Match(uuid=10853, description='Cycling Power Feature',
    uuid_type='characteristic')}
Got:
    {Match(uuid=10853, description='Cycling Power Feature', uuid_type='characteristic')}

@koenvervloesem
Copy link
Owner

koenvervloesem commented Dec 12, 2023

Yes, and the solution is the # doctest: +NORMALIZE_WHITESPACE I was talking about in #43 (comment)

So this example should look like:

>>> rl.lookup("Power Feature", uuid_types=['characteristic'],
... logic="SUBSTR")  # doctest: +NORMALIZE_WHITESPACE
{Match(uuid=10853, description='Cycling Power Feature',
uuid_type='characteristic')}

@vincentdavis
Copy link
Contributor Author

Thanks for your patience

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (59465e8) 100.00% compared to head (05a456e) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          108       153   +45     
  Branches        16        39   +23     
=========================================
+ Hits           108       153   +45     

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

@koenvervloesem
Copy link
Owner

Perfect! Now only the code coverage check complains, but the type checking line should be ignored. You can solve this by adding the following lines at the end of .coveragerc:


    # Assume if TYPE_CHECKING is covered
    if TYPE_CHECKING:

@koenvervloesem koenvervloesem merged commit 2b8cbd2 into koenvervloesem:main Dec 13, 2023
20 checks passed
@koenvervloesem
Copy link
Owner

Thanks for patiently fixing your code to follow the project's code style!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants