-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
@nomennescio
nomennescio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing additional libs from extra might be OK.
Other changes I disapprove of.
Kacarott
commented
Jan 29, 2024
Really all other changes?? Can you at least give some reasons? After all I have good reasons for each change.
nomennescio
commented
Jan 29, 2024
Really all other changes?? Can you at least give some reasons? After all I have good reasons for each change.
preloading is also probably a good idea; will look into that.
In discord nomen has said that I should explain why I want the changes rather than him needing to provide any justification for why he doesn't like them. I answered there, but also posting here for reference:
- Moving
imager.factorfromtestesttofactor
- The reason here feels obvious to me. As said above, it clearly belongs in
factor, and not intestest, as it is directly involved in creating the runner instance of factor for codewars, and has nothing to do withtestest. I assumed that the reason it was put intestestin the first place was simply because you previously didn't have access to thefactorrunner before, and thereforetestestwas the next best place, and so I didn't realise that this would be a point of contention.
- Preloading select vocabularies from
extras:
- See this issue, but you've also said that you can agree this is a problem, so I wont expand further here.
- Running
imager.factoras a one time script instead of installing as a vocabulary:
- It is run exactly to create the factor image and never again, and so has no purpose being a permanent vocabulary. It pollutes the vocabulary namespace, and also (very slightly) increases the size of the docker image. In general removing it makes the installation cleaner. These are fairly trivial issues, but it is also an extremely trivial fix.
- Removing additional broken vocabs:
- Since a handful of vocabs were already removed due to being useless and taking up memory space, a number of others vocabs which relied on these vocabs were then broken. This change removed those, then removed vocabs which relied on this, etc. until no more vocabs relied on something broken. Not only does this save memory, it also prevents users from trying to access vocabs which appear to exist, but then break under certain circumstances.
- Installs
math.marginstoworkinstead ofpre, including de-prioritising its loading:
math.marginsdoes not need to be loaded before other vocabs, doing so adds unnecessary complexitymath.marginsbeing in a prioritised root violates the factor principle of earlier loaded roots not relying on vocabs from later loaded roots. vocabs incoredo not rely on vocabs inbasisdo not rely on vocabs inextras.math.marginsbeing prioritised means that if in the future a differentmath.marginsvocabulary is added, then any vocabs in the monolib which rely on it will break, since they will load yourmath.marginsinstead. Causing the standard lib to break internally, is not a good thing.- Additionally, if a different
math.marginsvocabulary is added but no other library relies on it, then no error will be caused, but it will be silently shadowed, meaning that any users that try to use it in solutions will instead import the custommath.marginsinstead of the stdlib version. - If the intent is to provide an "early warning system" for future versions to inform that there is a name conflict, then adding an explicit test for that is a clearly better option, rather than relying on the standard lib potentially breaking during compilation.
- Working around the standard factor library structure and load order is a hack and bad practice, precisely because of the bugs it could introduce
nomennescio
commented
Jan 30, 2024
I object to 1, 3 and 5, because of how I see units of change related to testest and this archive, I don't want to change that.
That leaves 4 in this archive, and 2 to be addressed in testest.
I don't know what you mean by "units of change", however there is already precedent for moving docker related things to this repository, as the Dockerfile and many other files were already moved by kazk. There is no need for imager.factor to be in testest. Moving it breaks nothing, as nothing in testest relies on it.
It doesn't make any sense that testest needs to be updated and a new release created in order to fix issues with the codewars factor image that have nothing to do with testest. Keeping it this way, when we have a clear alternative, is artificially entangling dependencies.
Note I am specifically talking about point 1 for now, as it is directly related to point 2, and makes sense to resolve them together.
Kacarott
commented
Jan 30, 2024
About point 3, I would also like to know what benefits we gain by installing imager.factor as a vocabulary instead of running it as a one time script.
Kacarott
commented
Jan 30, 2024
I have created issues for each change in this PR, listing the motivation behind the changes. Later I will reduce this PR to focus only on points 1, 2 and 4, leaving 3 and 5 for further discussion.
Uh oh!
There was an error while loading. Please reload this page.
Details of changes in this branch:
testestwith a new one in this repository, which is a more appropriate location in my opinion. (solves Move imager.factor from testest to this repository #13 )coreandbasisas before, but also preloads a collection of useful/commonly used vocabs fromextras(this solves Slow load times for extras #11 )extraswhich were unusable due to relying on other vocabs which were already removed. (mostly games, demos, etc) (solves Remove vocabs that rely on already removed vocabs #15 )math.marginsfromtestesttoworkinstead of making a new, prioritised vocab rootpre. For 0.99 this should not change anything, but prevents breaking future versions if they add amath.marginsvocab. (solves Installmath.marginsto work instead of making a new prioritised root "pre" #16 )I would appreciate reviews from both @kazk and @nomennescio to make sure I haven't overlooked something important.