-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
Clarify package (re)loading use cases #1974
Comments
One of the early issues was that each instance of I believe that issue has been addressed. Consider for example:
$ ./sile repack.sil
SILE v0.14.14.r303-ga8f6166 (LuaJIT 2.1.1700206165) [Rust]
<repack.sil> as sil
<./core/sile.lua:444> as lua
table: 0x55d29d722590
<./core/sile.lua:444> as lua
table: 0x55d29d722590
[1] Note the table ID is the same even after reloading. Is the issue that we need to also stash the actual Penlight class instances for each module? It does appear that we are not doing that (I actually thought we were but couldn't find it right now). |
Script:
Commented output from SILE v0.14.16 (LuaJIT 2.1.1702233742)
None of this seems logical to me:
|
By the way your example above is slightly misleading:
|
And a "reasonable" use case.
Do you really think this is what people expect? The question is not related to my packages (whether they'd hack a You may say it's per design. I might say this is a wrong and broken paradigm. |
= "markdown" internally loads "url" (via |
Thanks for the feedback. There are some points here that take me by surprise because that is not what I thought was going on, and also a couple points that I disagree with (e.g. yes in some cases I expect the results you suggest are unexpected), but either way this gives me something to work through. |
One major issue I just uncovered is that |
@Omikhleia Have you used SILE |
How can this be tagged as a "documentation" (only) issue, since:
I had problem with this since July 2022 (#1366 (comment)), which was unanswered I then opened a dedicated issue in August 2022, #1531 which got closed though the above-mentioned concerns were unaddressed. I was bugged here in Dec 2022, and mentioned the multiple instantiation problem again in my feedback. I reported numerous related issues (let's pick just one, February 2023, #1708)... That were fixed without concern for the global re-instantiation picture. Honestly, my sile·x stuff was mostly created in Jul 30, 2023 because of this very pain point, as I wanted to remove hacks from other components. Sure, it then received other stuff (either in-advance experiments or backported PRs), but for everything new I've tried to play fair and also propose upstream updates too when I felt them to be ready. The multiple re-instantiation issue was the initial forking point. I was bugged again in Nov. 2023 -- though it does not fix anything... I was yet bugged again in Nov. 2023 for the very way sile·x worked -- and I addressed it, though it implies other workflows as described above might be affected. Frankly, I don't know what you expect me to do here -- I've said numerous times I was ill-at-ease with this behavior and could not find a way to sort it out. |
Erm, re-reading my previous answer, it might sound harsh, which wasn't the intent. |
It isn't. I also tagged it
todo
. As in it isn't a
bug
|
At this point the main gist of my expectation is in regard to this comment: Does the behavior in the develop branch slated to become v0.15.0 address your concerns or not? Your previous issue #1531 was closed by #1908, but your comments in this issue are evidently from the 0.14.x release series and still showing the old behavior. For example this MWE:
This is not the result you should be getting with the develop branch. I get this: While for this particular sample it gives the same results, for more advanced cases #1979 takes things a step farther and makes the new behavior more robust to allow reloads of modules to load the same instance as the original load (so that anything in the package's |
Quick answer: |
@Omikhleia Did you ever get a chance to test the develop branch in relation to package reloading to see if it actually addresses your usage concerns like I think it does? |
Originally posted by @Omikhleia in #1866 (comment)
I seem to have lost track about what the perceived the problem is with the current model. I know there were some hiccups even with core packages when we first changed up the loader, but I thought the issues were ironed out. Packages are allowed to accept initialization options, so blocking the initialization would block the most obvious user facing way to re-initialize with new options. As it stands all the core packages are idempotent (or something close) and this hasn't seemed to be an issue yet.
As for 3rd party packages that want to make a different choice and only allow loading once, I don't lee what is to stop them using the current model.
What is there left that is not covered by the current model of allowing re-initialization (either to correct things clobbered by other packages or use new options) and also allowing the package itself to do whatever it wants as far as whether to to actually do anything on initialization?
The text was updated successfully, but these errors were encountered: