core: make ImplicitBuilder public #1034
Conversation
superlopuh
requested review from
tobiasgrosser,
compor,
AntonLydike,
JosseVanDelm and
PapyChacal
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1034 +/- ##
==========================================
- Coverage 86.90% 86.89% -0.01%
==========================================
Files 125 125
Lines 19317 19319 +2
Branches 2929 2929
==========================================
+ Hits 16787 16788 +1
- Misses 2028 2029 +1
Partials 502 502
☔ View full report in Codecov by Sentry. |
webmiche
added
the
minor
| @@ -114,6 +114,9 @@ def region( | |||
| else: | |||
| return Builder._region_args(input) | |||
|
|
|||
| def implicit(self) -> ImplicitBuilder: | |||
There was a problem hiding this comment.
Can you add here a reference to ImplicitBuilder as documentation?
There was a problem hiding this comment.
I'm a bit confused, it's a command/control click away
There was a problem hiding this comment.
Well yes, but at this point, especially for new people, what people see is just a function creating something else. They don't necessarily know how to use it, and having just a bit of documentation can help.
Just something like "Create an implicit builder that is used in a with region. See ImplicitBuilder" can really help people understanding what this is supposed to be. Otherwise, people might just skip over it?
What do you think?
There was a problem hiding this comment.
Wouldn't your first instinct be to look at what the return type is? I doubt that many people will skip over it, although maybe I'm wrong. In VSCode you wouldn't even need to click on it, hovering over the type will bring up the docstring
There was a problem hiding this comment.
Well, yeah when you are looking at the operation.
But when you are looking at a use of implicit, you would like to know what it does (meaning, that it creates a new implicit context). Here, you would have to jump to the implicit definition to understand what it does, and then to jump to the ImplicitBuilder.
That's why I would say that a single sentence would still help, but if you think I'm too hard on that, we could skip it.
There was a problem hiding this comment.
No I think you're right, but in some ways doccomments are a liability, in that there's no way to test that they don't become outdated as we change the code. I'd like to keep documentation as close as possible to the place where the code is defined, so that it's easier to update things as they evolve, and avoid misleading people. I think we can assume that the kind of people who read documentation can navigate to the definition of a class they are interested in. Especially since I don't expect that this function will be used explicitly. Now that I think about it, maybe I should just delete this function and use ImplicitBuilder(block) instead... I'll try it out.
There was a problem hiding this comment.
I deleted this method in the other PR
|
Sorry @math-fehr was a bit trigger happy with the merge, lmk if there's documentation that you think is missing, I'm not exactly sure what you meant still. |
From the discussion on #1017, I thought it might be better to make the ImplicitBuilder public in a separate PR.