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

Change import to using in test #83

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

clizbe
Copy link
Member

@clizbe clizbe commented Dec 6, 2024

Related issues

Closes #42

Checklist

  • I am following the contributing guidelines
  • Tests are passing
  • Lint workflow is passing
  • Docs were updated and workflow is passing

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.91%. Comparing base (6bc433b) to head (7100bd9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage   66.91%   66.91%           
=======================================
  Files           6        6           
  Lines         266      266           
=======================================
  Hits          178      178           
  Misses         88       88           

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

@clizbe clizbe requested a review from suvayu December 17, 2024 16:08
@suvayu
Copy link
Member

suvayu commented Jan 8, 2025

Ah, I think you tagged me for review just after I went on holiday :-p

I don't recollect exactly, but wasn't the convention to not use using Module but using Module: name1, name2, ... instead? The rationale being, the first form imports everything into the namespace, whereas the second only imports the names we need explicitly; explicit > implicit.

@clizbe
Copy link
Member Author

clizbe commented Jan 10, 2025

@abelsiqueira
I can't remember either. I just followed what I saw in another repo and tried to make it work. (I don't understand the difference between the methods.)

@suvayu
Copy link
Member

suvayu commented Jan 10, 2025

IIRC, import name means now you can define overloads for name (i.e. type dispatch), but using prevents that. So doing using is preferred unless you are extending something. But only doing using module imports all exported names from the module, which we don't want since that's implicit.

@clizbe
Copy link
Member Author

clizbe commented Jan 10, 2025

Here's the full conversation:
TulipaEnergy/TulipaEnergyModel.jl#431

Here's the decision.

After some exploration, I suggest using Package: Package, some_stuff.

  • using Package: Package will import the module name, so it can be used for prefixing non-imported stuff.
  • using Package: some_stuff will load some_stuff, which will permit using some_stuff without the prefix. I recommend this for obvious cases (such as JuMP's macros), or functions with explicit names (can't think of any right now).
  • For some stuff, don't explicitly load it. Instead, when using it, prefix with the package name. For instance, Graphs.edges.

@clizbe
Copy link
Member Author

clizbe commented Jan 10, 2025

@abelsiqueira @suvayu
Maybe we should make an exception for TulipaIO and TulipaEnergyModel, with:

import TulipaIO as TIO
import TulipaEnergyModel as TEM

Or is that extra confusing? Just asking because the names are quite long otherwise and I don't think you can use as with using.

@clizbe
Copy link
Member Author

clizbe commented Jan 10, 2025

For now I'm leaving it explicit at TulipaIO.whatever which I think is actually nice - at least in the tests.

@suvayu suvayu merged commit 92895d3 into TulipaEnergy:main Jan 10, 2025
5 checks passed
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.

Change 'import' to 'using' in the tests
2 participants