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

Rename assume! to assume #12

Closed
jariji opened this issue Feb 25, 2024 · 4 comments
Closed

Rename assume! to assume #12

jariji opened this issue Feb 25, 2024 · 4 comments
Labels
UX Issues relating to UX

Comments

@jariji
Copy link
Contributor

jariji commented Feb 25, 2024

https://docs.julialang.org/en/v1/manual/style-guide/#bang-convention gives the convention

Append ! to names of functions that modify their arguments

Since assume!(::Bool) doesn't modify its arguments I suspect it should be assume(::Bool).

@Seelengrab
Copy link
Owner

Seelengrab commented Feb 25, 2024

Unlike rand (where the hidden state is always assumed to exist), the currently used test case does not always exist, so the bang serves as a reminder that there is some (hidden) state being modified when this is called. I'm feeling a bit uneasy about removing the bang there (and similarly with target! and reject! too) because of that.

It's a bit like Random.seed!(), which seeds a (hidden) default.

@Seelengrab Seelengrab added the UX Issues relating to UX label Feb 25, 2024
@jariji
Copy link
Contributor Author

jariji commented Feb 29, 2024

I like having a simple rule with a specific universal documented meaning so authors can use it to communicate properties of a function simply and precisely to their users. Unfortunately the current "convention" doesn't follow the rule quoted above; it's subjective, as seen in idiosyncrasies like rand(rng), print(::IOBuffer), and seed!(k), which makes it hard to use for precise communication between individuals.

Anyway that's just my personal rant, not your problem. Feel free to close this if you like.

@Seelengrab
Copy link
Owner

I'll do a small export-audit of everything that I consider API before registering, so I'll take a look at this then.

@Seelengrab
Copy link
Owner

Alright, I've reworked the exposed API. Everything now consistently uses <verb>! from the user-side, to indicate the change of hidden state. I've also modified the internal uses to follow the usual convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX Issues relating to UX
Projects
None yet
Development

No branches or pull requests

2 participants