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 a lazy is_sorted function to collections #159

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Add a lazy is_sorted function to collections #159

merged 5 commits into from
Jul 15, 2024

Conversation

clintval
Copy link
Member

No description provided.

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.44%. Comparing base (fbe5923) to head (9eadbdf).

Files Patch % Lines
fgpyo/collections/__init__.py 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
+ Coverage   88.38%   88.44%   +0.06%     
==========================================
  Files          16       16              
  Lines        1773     1791      +18     
  Branches      377      381       +4     
==========================================
+ Hits         1567     1584      +17     
  Misses        136      136              
- Partials       70       71       +1     

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

IterType = TypeVar("IterType")

_LessThanOrEqualType = TypeVar("_LessThanOrEqualType", bound=_SupportsLessThanOrEqual)
Copy link
Member

Choose a reason for hiding this comment

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

Is it appropriate for this type be "private" when it is used as a type parameter in a "public" method below?

Copy link
Member Author

@clintval clintval Jul 15, 2024

Choose a reason for hiding this comment

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

Can do!

I notice fgpyo doesn't use __init__.py for public namespace definition which makes defining a public API trickier. For example, the function pairwise in this PR is now exposed as our public API instead of just as an import or a helper. I do see Matt is tracking that issue here, though:

EDIT: I renamed pairwise to _pairwise to avoid this without the need to restructure the project.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, one way to manage what is public versus private is #136, but another way to solve it is to the way we're doing here. Moving things out of __init__.py into submodules doesn't solve this, since the methods/classes can be public there, but it does help with namespace issues. Lets move that discussion to #136 since the namespace issue is compelling, and any new method can always be private like we have here for pairwise.

@clintval clintval temporarily deployed to github-actions-ci July 15, 2024 19:13 — with GitHub Actions Inactive
Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +41 to +47
- pymdownx.highlight:
anchor_linenums: true
line_spans: __span
pygments_lang_class: true
- pymdownx.inlinehilite
- pymdownx.snippets
- pymdownx.superfences
Copy link
Member Author

Choose a reason for hiding this comment

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

Now all example code blocks are code-colored too!

Screenshot 2024-07-15 at 11 23 50 AM

@clintval clintval merged commit 2261825 into main Jul 15, 2024
8 checks passed
@clintval clintval deleted the cv_is_sorted branch July 15, 2024 21:10
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.

4 participants