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

docs: Expand processor/provider docs #98

Merged
merged 4 commits into from
Dec 17, 2023
Merged

Conversation

lucyleeow
Copy link
Contributor

Expand the processor/provider docs in 'getting started'. In particular how the store iterates over processors/providers.

Hopefully it is correct 😬

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (29210cf) 100.00% compared to head (ad192d7) 100.00%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #98   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          521       521           
=========================================
  Hits           521       521           

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

Copy link

codspeed-hq bot commented Dec 13, 2023

CodSpeed Performance Report

Merging #98 will not alter performance

Comparing lucyleeow:doc_inj (ad192d7) with main (29210cf)

Summary

✅ 5 untouched benchmarks

@tlambert03 tlambert03 changed the title DOC Expand processor/provider docs docs: Expand processor/provider docs Dec 16, 2023
Comment on lines 121 to 123
if hasattr(thing, name):
return thing.name
return None
Copy link
Member

Choose a reason for hiding this comment

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

if we're going to add this, then the return type annotation should be changed to str | None. But I also feel like it's a bit contrived here... since Thing always has an attribute name... so it's not a great example of a processor that may or may not return a value (like, for example napari.get_current_viewer). So, if you think an example of this is needed (in addition to the text above in the "multiple providers" note) then let's add a new section with a new provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah examples are hard. I don't think it's needed, the doc should be enough

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

Thanks @lucyleeow! Yep everything is pretty much correct! I made a couple slight wording changes and elaborations. One more comment left to discuss then lets merge.

@lucyleeow
Copy link
Contributor Author

lucyleeow commented Dec 16, 2023

Oh one more thing I wanted to mention is that if a default value is provided, no provider will attempt to inject. It seems obvious but e.g. if you have Optional[sometype] = None, injection wouldn't be done.

@tlambert03
Copy link
Member

yep! Good thing to add

@tlambert03
Copy link
Member

(if you ever run into a case where you need/want to be able to override default values with injected values, that would be a reasonable feature request, probably added as a parameter to inject)

docs/getting_started.md Outdated Show resolved Hide resolved
@tlambert03 tlambert03 enabled auto-merge (squash) December 17, 2023 14:31
@tlambert03 tlambert03 merged commit 2044fb3 into pyapp-kit:main Dec 17, 2023
20 of 21 checks passed
@lucyleeow lucyleeow deleted the doc_inj branch December 17, 2023 23:03
@lucyleeow
Copy link
Contributor Author

Thanks for making the changes! I didn't have time to add a note on default values so did it here: #99

@tlambert03 tlambert03 added the documentation Improvements or additions to documentation label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants