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

Real fn renames and Eq refactor #305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dragazo
Copy link
Contributor

@dragazo dragazo commented Jul 26, 2024

Note: this PR contains breaking changes, so if merged we should at least change the minor version number before next release

When using this crate, I thought it was confusing that operators are used for symbolic arithmetic but doing the same for checking equality is non-symbolic (ast comparison). To actually check symbolic equality, you need to use _eq(other), which looks weird compared to other symbolic inequality checks like le(other) or gt(other). So in this PR I've renamed _eq to eq, the only cost of which is removing the PartialEq trait impl from the concrete Ast types. Instead, I've added a new method to Ast called ast_eq(&self, other: &dyn Ast) -> bool that does this check explicitly. I think this is less likely to be used accidentally than ==, for which the user probably wants symbolic equality anyway.

Also, there were several functions for Real that take/return a fraction but use the term "real" (e.g., from_real(i32, i32)). I've renamed these to "rational".

Also, for the new from_rational function (replacing from_real), I changed the input types to i64 instead of i32, but the deprecated wrapper still uses i32. This is to match the return type of as_rational (new name for as_real but otherwise unchanged).

For all of the function renames, I kept a shallow wrapper with the old name and just marked it as deprecated. So the renames aren't a breaking change, but removing the PartialEq impl is.

@waywardmonkeys
Copy link
Contributor

Thanks!

I think this sounds like an improvement!

If you could:

  • Rebase on top of master
  • Squash your commits into a single one
  • Make the commit message for that commit be like your description of this PR.

That would be great!

Rename `_eq` to `eq` for consistency with `le`, `ge`, etc.

Remove `PartialEq` and `Eq` from concrete `Ast` types to avoid accidental use.

Add `ast_eq` to `Ast` to allow for explicit ast equality checks.

Rename some `Real` methods involving fractions to "rational" instead of "real".

Extend `Real::from_rational` (prev. `Real::from_real` to take `i64` for num/den).
@dragazo
Copy link
Contributor Author

dragazo commented Jul 26, 2024

Sure, I just merged and squashed it back down to one commit.

@dragazo dragazo mentioned this pull request Aug 2, 2024
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.

2 participants