-
Notifications
You must be signed in to change notification settings - Fork 1
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
Install math.margins
to work instead of making a new prioritised root "pre"
#16
Comments
As also explained in #13, testest has a unique relationship to the Factor image, as it needs to integrate an external repository into a monorepo of Factor, specifically also to gain indepedent release cycles from the Factor releases. This is especially important as Factor currently doesn't have a reliable release plan, and releases are very infrequent (last gap between releases was 5 years). It is not acceptable to use unreleased Factor code. However, it might be needed in special circumstances to fix an existing Factor image. The role of the |
This is done trivially by adding the vocab to
If
I stated the load order because that is all that it is affecting. You manually insert
It does not need to have a special role, it is a utility vocab just like any other. And having it in
I think they definitely do. There is no need for Since I think I have shown that quite a few of the stated aims of your design here are not actually achieved, I think that is grounds for the issue to be reopened and the design re-examined. |
Currently
math.margins
is installed in a new vocabulary root "pre", which is specifically given priority loading during imaging. There are a couple problems with this, listed below.math.margins
should be installed following regular factor practice, in the "work" root (same place astestest
is installed).The problems:
math.margins
does not need to prioritised loading, doing so adds unnecessary complexitymath.margins
being in a prioritised root violates the factor principle of earlier loaded roots not relying on vocabs from later loaded roots. vocabs incore
do not rely on vocabs inbasis
do not rely on vocabs inextra
.math.margins
however imports vocabs fromcore
andbasis
, but is loaded before them.math.margins
being prioritised means that if amath.margins
vocabulary is added to the standard library in the future, then any vocabs in the standard library which rely on it will break, since they will load the externalmath.margins
instead. Causing the standard lib to break internally, is not a good thing.math.margins
vocabulary is added but no other vocabulary relies on it, then no error will be caused during docker image creation, but it will be silently shadowed. This means that any users that try to use it in solutions will instead import the externalmath.margins
instead of the expected standard library version.In summary, the current behaviour is bad practice, and is actively creating potential future compatibility issues. On top of that, it is unnecessary -
math.margins
will work exactly the same from "work" as it does from "pre".If the intent in this design 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.
The text was updated successfully, but these errors were encountered: