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

Apply PEP561 #520

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

phi-friday
Copy link
Contributor

closes: #493
PEP561: https://peps.python.org/pep-0561/

Based on the previous discussion, type hinting using TypeVar was ruled out as it would be a barrier to entry for casual users. Instead, #507 was merged in.

In my opinion, this library fully meets the needs of the existing contributors and users,
so it makes sense to add py.typed to let users know that this library provides sufficient type hints.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.31%. Comparing base (aed6a25) to head (5069ca9).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   96.19%   96.31%   +0.12%     
==========================================
  Files          10       11       +1     
  Lines         867      869       +2     
==========================================
+ Hits          834      837       +3     
+ Misses         33       32       -1     

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

Copy link
Member

@till-m till-m left a comment

Choose a reason for hiding this comment

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

How do we check the effect of this change?

pyproject.toml Outdated
packages = [{include = "bayes_opt"}]
packages = [
{include = "bayes_opt"},
{include = "bayes_opt/py.typed"},
Copy link
Member

Choose a reason for hiding this comment

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

It seems this explicit include might not actually be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In local, I ran the poetry build command to verify that py.typed was included.
But I prefer to be explicit rather than implicit.
Of course, this is just my personal preference, but if you prefer simplicity, it seems better to remove it.
I'll fix it as soon as you point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it

@till-m till-m mentioned this pull request Sep 20, 2024
1 task
@till-m till-m merged commit 9324da3 into bayesian-optimization:master Sep 20, 2024
11 checks passed
@phi-friday phi-friday deleted the fix-apply-pep561 branch September 20, 2024 08: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.

support PEP 484
2 participants