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

Update package to use package extensions #32

Closed

Conversation

thazhemadam
Copy link

@thazhemadam thazhemadam commented Nov 3, 2023

Update the package to use package extensions, instead of explicitly depending on all the respective packages it supports.
The package still at a high level remains the same interface wise and this makes it a lot lighter.

Update the expected error message to match the actual error message
thrown while Julia versions ≥ 1.8.
@thazhemadam thazhemadam force-pushed the at/use-package-extensions branch 2 times, most recently from f65e0a7 to 39b124e Compare November 3, 2023 12:15
Since AtomsIO is meant to be a standardized package that allows us to
read/write atomic structures, it makes natural sense that all the
different packages it supports are made package extensions and aren't
direct dependencies unnecessarily making the package bigger than it
needs to be.
@mfherbst
Copy link
Owner

mfherbst commented Nov 8, 2023

Thanks for the PR. The first fix (for the tests) I would take immediately.

For the second aspect (splitting into extensions). I'm not sure I agree. Do you have a use case in mind that requires you to do this or where you have great disadvantages without using package extensions ?

The original rational to not use package extensions is:

  • To me AtomsIO is a "user-facing package", i.e. a package mostly meant for interactive explorations and small scripts and not for speed, so the compromise is overall more towards convenience rather than being lightweight. If you want a lightweight way of loading files, you could use the dependent packages directly, which by now all support AtomsBase in some form or another. In case there is a need for a common interface that is as lightweight as possible, we can think about how to get there, but this is tricky with package extensions also for other reasons ...
  • Some formats are supported by multiple packages, but since formats are underspecified not all packages support everything and moreover as a result of the underspecification writing format A with Package X and reading it with Package Y can lead to issues. In AtomsIO we ensure that the same packages are always tried in the same order, such that you are guaranteed that a save_system to format A followed by a load_system from format A is as lossless as possible. With using package extensions the behaviour of load_system / save_system depends on what packages are loaded, which can lead to subtle inconsistencies in the data or the simulation, which in my opinion should be avoided as much as possible.
  • Keeping an explicit overview over the packages that exist for parsing files and which respective file formats they support is tedious. With package extensions I would need to keep track of this and do an explicit using PkgX matching the file type I have at hand. Having AtomsIO depend explicitly, it is a superpackage which just tries all possible packages, without me having to worry.

@mfherbst
Copy link
Owner

Closing as discussion has stalled.

@mfherbst mfherbst closed this Oct 22, 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