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

Aqua + typos CI #253

Closed
wants to merge 2 commits into from
Closed

Aqua + typos CI #253

wants to merge 2 commits into from

Conversation

ArnoStrouwen
Copy link
Member

Aqua found possible type piracy.
Should those methods be upstreamed?
total_degree, vars

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7550ac5) 89.90% compared to head (e496ff0) 74.47%.

Files Patch % Lines
...rc/RationalFunctionFields/RationalFunctionField.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #253       +/-   ##
===========================================
- Coverage   89.90%   74.47%   -15.43%     
===========================================
  Files          25       25               
  Lines        3208     3205        -3     
===========================================
- Hits         2884     2387      -497     
- Misses        324      818      +494     

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

@sumiya11
Copy link
Collaborator

sumiya11 commented Dec 3, 2023

Hi, thanks for the PR.
I think these type piracies are fine.
In addition, I suggest beautiful_generating_set instead of beautifully_generators/beautifuly_generators.
@pogudingleb

@ChrisRackauckas
Copy link
Member

I don't think so. Why wouldn't those two methods get upstreamed? They seem like generally useful functions for the package that defines the algebraic types?

@pogudingleb
Copy link
Collaborator

@ArnoStrouwen Thanks!

In addition, I suggest beautiful_generating_set instead of beautifully_generators/beautifuly_generators.
@pogudingleb

I also like this better. Another option would be to simplify_generators

Why wouldn't those two methods get upstreamed?
They seem like generally useful functions for the package that defines the algebraic types?

I agree with Chris, at least for vars, I have created an issue for this.

@sumiya11
Copy link
Collaborator

sumiya11 commented Dec 3, 2023

Right, I agree, we can rename total_degree, and vars can perhaps be defined in AbstractAlgebra.jl. I will open an issue there once I land

@pogudingleb
Copy link
Collaborator

Right, I agree, we can rename total_degree, and vars can perhaps be defined in AbstractAlgebra.jl. I will open an issue there once I land

I guess, they would rather appreciate a PR)

@pogudingleb
Copy link
Collaborator

Superseded by #274

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.

4 participants