codewars / factor

Container image for Factor
MIT License
1 stars 1 forks source link

Install `math.margins` to work instead of making a new prioritised root "pre" #16

Open Kacarott opened 7 months ago

Kacarott commented 7 months ago

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 as testest is installed).

The problems:

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.

nomennescio commented 7 months ago

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 pre vocab root is to have control over the search order of vocabularies, not the load order of vocabularies as you incorrectly state (although when building the image it is explicitly loaded as are the other libraries, but at that time all other libraries from core and basis most likely have already been loaded). Having math.margins in pre signals its special role and it also makes sure it doesn't get overloaded by any Factor code in Codewars, and therefore prevents accidental name clashes or cheating. Your points about "regular Factor practice" don't apply for this situation. As I explained in #13, no testest version will be released without it being tested by me, and I will catch any potential nameclashes and deal with that accordingly, so I see no issue there.

Kacarott commented 7 months ago

as it needs to integrate an external repository into a monorepo of Factor

This is done trivially by adding the vocab to work.

However, it might be needed in special circumstances to fix an existing Factor image

If pre is ever needed in the future to fix something, (which I highly highly doubt) then we can just implement it then? That is not a reason why it should exist now, nor why math.margins should be in it.

The role of the pre vocab root is to have control over the search order of vocabularies, not the load order of vocabularies as you incorrectly state

I stated the load order because that is all that it is affecting. You manually insert pre as first position in the vocab roots, then load them in that order, so it affects load order. Many core and basis vocabs might be already loaded, but not all. But the search order, as defined by the vocab roots, is not saved in the image. See here notice how pre is not listed in the vocab roots. So, if your goal is the load order, then it has the issues I mentioned above. If your goal is the search order, then this is not a valid solution to achieve that. (nor do I think it is necessary, as I will show below).

Having math.margins in pre signals its special role and it also makes sure it doesn't get overloaded by any Factor code in Codewars,

It does not need to have a special role, it is a utility vocab just like any other. And having it in pre does not ensure it doesnt get overloaded by any Factor code, it is actually quite trivial to overload it and the definitions in it: see here. Codewars attitude regarding cheating has always been that it is not worth trying to go to great lengths to prevent it. People who manipulate this to cheat will simply be found by mods and punished as cheaters.

Your points about "regular Factor practice" don't apply for this situation.

I think they definitely do. There is no need for pre to exist currently, let alone for math.margins to be in it. math.margins would work just as well in work just as testest does. And while there is no need to break conventions and change the behaviour of Factor by modifying the load order, then we shouldn't.

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.