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

Centralize import/export statements and clean up main file #1537

Merged
merged 13 commits into from
Apr 4, 2024

Conversation

lgoettgens
Copy link
Collaborator

@lgoettgens lgoettgens commented Dec 21, 2023

Nemocas/Nemo.jl#1601 for AbstractAlgebra

Edit: some parts have already been merged as #1556

@lgoettgens
Copy link
Collaborator Author

Most of the work was already done by @fingolfin in #1281.

Some open questions on how to proceed:

  1. Why are there two long export lists in AbstractAlgebra.jl (one starting around line 220, the other around line 790)?
  2. For the submodules (Generic and PrettyPrinting) there is currently a mix of exporting their functions from the submodule and importing into the main AbstractAlgebra module. Which of the two is the one to use?

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 86.69%. Comparing base (435ec91) to head (1fff81a).
Report is 4 commits behind head on master.

Files Patch % Lines
src/WeakValueDict.jl 88.88% 1 Missing ⚠️
src/julia/Matrix.jl 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1537      +/-   ##
==========================================
- Coverage   86.76%   86.69%   -0.08%     
==========================================
  Files         114      114              
  Lines       29696    29652      -44     
==========================================
- Hits        25767    25707      -60     
- Misses       3929     3945      +16     

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

@fingolfin
Copy link
Member

  1. Why are there two long export lists in AbstractAlgebra.jl (one starting around line 220, the other around line 790)?

Probably historic reasons? I would try to merge everything into the first list and see what happens.

  1. For the submodules (Generic and PrettyPrinting) there is currently a mix of exporting their functions from the submodule and importing into the main AbstractAlgebra module. Which of the two is the one to use?

Maybe I am missing something in your question, but I don't think the two are interchangeable in general?

  • if foo is defined before module Generic is loaded, then Generic must import it
  • if foo is only defined *after module Generic, then Generic cannot import it; and if code outside Generic is supposed to use foo or even install methods for it, then Generic but must export it

@lgoettgens
Copy link
Collaborator Author

  1. For the submodules (Generic and PrettyPrinting) there is currently a mix of exporting their functions from the submodule and importing into the main AbstractAlgebra module. Which of the two is the one to use?

Maybe I am missing something in your question, but I don't think the two are interchangeable in general?

  • if foo is defined before module Generic is loaded, then Generic must import it
  • if foo is only defined *after module Generic, then Generic cannot import it; and if code outside Generic is supposed to use foo or even install methods for it, then Generic but must export it

I am both times talking about the flow Generic -> AbstractAlgebra. Sometimes, Generic exports stuff to AbstractAlgebra and sometimes AbstractAlgebra imports stuff from Generic.

@fingolfin
Copy link
Member

Maybe wait a bit with this, as the GroupsCore PR is getting closer to being finished and it'll touch the exports a bit, too, so there are conflicts

@fingolfin
Copy link
Member

Ah, so I indeed misunderstood, sorry. Well, we never do using .Generic, do we? So I would expect all those export statements in Generic to have no effect?

@lgoettgens
Copy link
Collaborator Author

Ah, so I indeed misunderstood, sorry. Well, we never do using .Generic, do we? So I would expect all those export statements in Generic to have no effect?

Ah true... So I will delete them (without semantical change)

@fingolfin
Copy link
Member

@lgoettgens this can in principle be updated and merged, right? Nothing I should do, is there?

I realize you may have more important things to do right now, I just want to make sure that I am not unwittingly the blocker :-).

@lgoettgens
Copy link
Collaborator Author

I think there is basically nothing really no to do here (apart from merging the two export lists). But as long as this is not compatible with Oscar master, I am afraid to break things due to the OscarCI not running. So I would only start to work on this after oscar-system/Oscar.jl#3231

@lgoettgens lgoettgens force-pushed the lg/exports branch 4 times, most recently from b9ed1e0 to 51d2751 Compare April 3, 2024 14:42
@lgoettgens lgoettgens changed the title Centralize import/export statements Centralize import/export statements and clean up main file Apr 4, 2024
@lgoettgens lgoettgens force-pushed the lg/exports branch 2 times, most recently from 8b52437 to e82adc4 Compare April 4, 2024 09:19
@lgoettgens lgoettgens marked this pull request as ready for review April 4, 2024 12:00
@fingolfin fingolfin merged commit 95bd006 into Nemocas:master Apr 4, 2024
30 of 31 checks passed
@lgoettgens lgoettgens deleted the lg/exports branch April 4, 2024 12:51
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.

None yet

2 participants