-
Notifications
You must be signed in to change notification settings - Fork 7
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
Minor issues I found when reviewing the new 7.0 API #175
Comments
I do think a custom exception class does make the error more immediately clear than a generic Exception with message. But I don't feel strongly about it, and indeed you're not going to be catching it so a custom class certainly isn't required. Thoughts @PietroPasotti ?
I had wondered about this too - they don't offer a huge amount over just doing the
The use-case is that it makes it easier to use I had originally thought the same applied to notices, e.g. you set up a notice and then you want to do
There's a lot less to a
Is that fixed in #169? It's a bit vague for me to be sure, but I would guess so - I read over the actual docs there reasonably carefully (although still missed the State.status example above). Dora is also going to review that PR so will hopefully catch anything bad.
Good question. Thoughts @PietroPasotti ?
I was looking at this in #169 also, because there's currently this slightly odd situation where almost everything that is public is in the top-level scenario namespace. However:
|
Weak preference for custom exceptions just because it makes it more immediately clear from the exception name what the error is about.
That was meant as syntactic sugar for doing variations of the
I think the use case is still present. We could hide them if we were to expose a
It will force people to pass around the reference to the output state as returned from
Yes.
Yes. Although one could argue that not being in the
Fine with this.
Not sure what's the best practice here. It'd be odd to have stuff like this in toplevel scenario. We could consider putting them in a types.py module, and were one to need them, they'd have to import
I don't see anyone actually importing this, but if it's for consistency...?
+1
yeah I don't think anyone will want to directly invoke the consistency checker unless you're doing fancy stuff like filtering out automatically-generated state/event combinations like I was trying to do in the hypothesis branch. Fine with 'hiding it'. Side-thought: perhaps we should add a
I think we could remove the module altogether and put the context manager in runtime.py (IIRC that's where it's used?) |
I think it was "this a good idea that we have no time to figure out now but should look into later".
👍
I agree for relations, but do you think there is for notices and storage? Storage seems too simple, and notices don't seem to fit because of the way Pebble collapses them into a single notice (per type/key).
In, say, a
Hmm,
I guess it's most likely to be used for types as well. I also don't see anyone pre-populating the Context with log lines.
Do you have any sense of how often people have to do this? I guess the only cases you know about would be when someone asks about something and you have to suggest disabling it temporarily because it's a bug.
👍
👍 |
Ok then let's add this to the 'reasons why that's a good idea list'.
Mm yes, it's only relations.
And that, indeed, is where this was getting at.
Nope, that's not something we should allow/encourage.
Yes that is the only use case. In principle the checker is there to help you ensure you're not making up juju-impossible scenarios, but imagine the frustration if you can't turn it off if you KNOW it's a false positive. |
A collection of small changes that generally fall into "remove x from the public API": * Remove the `with_can_connect`, `with_leadership`, and `with_unit_status` convenience methods from `State`. * Makes `next_relation_id`, `next_action_id`, `next_storage_index`, and `next_notice_id` private. * Removes `Context.output_state`. * Makes all the *_SUFFIX constants private. * Makes all the *_EVENTS constants private, except `META_EVENTS`, which is removed. * Makes `capture_events` private (and consolidates capture_events.py into runtime.py). * Makes both `hook_tool_output_fmt` methods private. * Makes `normalize_name` private. * Moves all of the Scenario error exception classes (the ones that no-one should be catching) to a scenario.errors namespace/module. * Renames the consistency checker module to be private. * Makes `DEFAULT_JUJU_VERSION` and `DEFAULT_JUJU_DATABAG` private. * Adds various classes/types to the top-level scenario namespace for use in type annotations: * Removes `AnyRelation` in favour of using `RelationBase` * Removes `PathLike` in favour of `str|Path`. Fixes #175.
Just opening an issue here for lack of a better place. The 7.0 API is looking really good IMO! Here are the (minor) issues I found:
ops-scenario/scenario/state.py
Line 373 in 602f9b9
The text was updated successfully, but these errors were encountered: