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

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

Open
Kacarott opened this issue Jan 30, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@Kacarott
Copy link
Collaborator

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:

  • math.margins does not need to prioritised loading, doing so adds unnecessary complexity
  • math.margins being in a prioritised root violates the factor principle of earlier loaded roots not relying on vocabs from later loaded roots. vocabs in core do not rely on vocabs in basis do not rely on vocabs in extra. math.margins however imports vocabs from core and basis, but is loaded before them.
  • math.margins being prioritised means that if a math.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 external math.margins instead. Causing the standard lib to break internally, is not a good thing.
  • Additionally, if a standard library 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 external math.margins instead of the expected standard library version.
  • Working around the standard factor library structure and load order is a hack and bad practice, precisely because of the bugs it could introduce

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.

@Kacarott Kacarott added the bug Something isn't working label Jan 30, 2024
@nomennescio
Copy link
Collaborator

nomennescio commented Feb 6, 2024

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
Copy link
Collaborator Author

Kacarott commented Feb 6, 2024

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.

@Kacarott Kacarott reopened this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants