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

Add divexact!, lcm!, submul! #1812

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Conversation

fingolfin
Copy link
Member

Also refactor the code defining them to use macros to reduce
repetition substantially.

Finally merge and update the sections discussion unsafe operators
in the "rings" and "ring interface" chapters of the manual: the latter
now

Also refactor the code defining them to use macros to reduce
repetition substantially.

Finally merge and update the sections discussion unsafe operators
in the "rings" and "ring interface" chapters of the manual: the latter
now
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (efb6ceb) to head (9732c44).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/fundamental_interface.jl 77.77% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1812      +/-   ##
==========================================
+ Coverage   88.03%   88.04%   +0.01%     
==========================================
  Files         119      119              
  Lines       30057    30045      -12     
==========================================
- Hits        26460    26454       -6     
+ Misses       3597     3591       -6     

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

src/exports.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

apart from the comments, fine with me. I am not a big fan of @eval for these small loops, but we can of course unroll them again later, if we want to change only one of them

@fingolfin
Copy link
Member Author

Comments addressed.

I don't love @eval for this either (it makes it harder to grep for declarations) I went for it here because I want to have the argument names consistent (z,a,b) and also the texts and there was a ton of repetition which is also error prone. In the end this saved me a lot of time and is more likely to be correct. If someone wants to unroll it again down the road, I don't mind, as long as I don't have to update the duplicated docstrings and implementations again later on :-).

The current setup also makes it super easy to add more of these... e.g. remove! and valuation! might be candidates. (Also set!(a,b) but we don't have a eval loop for that -- yet)

@lgoettgens
Copy link
Collaborator

Can you add HeckeCI, NemoCI and SingularCI to the "required" list? I am afraid to use auto-merge otherwise as it could just ignore those CI results.

@lgoettgens lgoettgens merged commit 2c9570b into master Sep 27, 2024
29 of 30 checks passed
@lgoettgens lgoettgens deleted the mh/more-inplace-fundamentals branch September 27, 2024 15:34
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