links icon indicating copy to clipboard operation
links copied to clipboard

Moved dependency chasing to the loader and fixed some tests

Open rajdakin opened this issue 7 months ago • 7 comments

This PR:

  • removes some ignore lines in the tests file for tests that are now passing;
  • fixes some tests;
  • moves the dependency chasing to the loader (as the comment suggests) so that we can search for files next to the current one (i.e. while compiling example/foo.links, the module Bar will be searched in example/bar.links instead of bar.links);
  • adds the case otherwise -> construct as an alias to the case _ -> construct;
  • adds a message to the testing output in case a test that was skipped is now passing.

rajdakin avatar Apr 11 '25 17:04 rajdakin

I don't know the context of this patch. What's the motivation? It seems to bundle a bunch of unrelated things together. Specifically, what's the motivation for the case change?

  • adds the case otherwise -> construct as an alias to the case _ -> construct;

dhil avatar Apr 12 '25 09:04 dhil

There are three tests that use this construct (examples/toggle.links, examples/handlers/toggle.links, and examples/handlers/toggle_web.links). As a side note, otherwise is tokenized to the OTHERWISE token, which only appears once in the entire parser in a location unrelated to this, so the change is safe.

The motivation of the entire PR is to make sure the tests are up-to-date, i.e. that tests which should pass do actually pass, that tests that pass are not ignored, and to fix minor issues (the case otherwise).

rajdakin avatar Apr 13 '25 12:04 rajdakin

The motivation of the entire PR is to make sure the tests are up-to-date, i.e. that tests which should pass do actually pass, that tests that pass are not ignored, and to fix minor issues (the case otherwise).

I think Daniel's point is that unlike fixing tests, this is a language change, and deserves to be pulled out and discussed/agreed sepatrately, as it does not appear to be an already recognized issue that no one had gotten around to yet.

I'm not sure what is wrong with case _, or what is less wrong with case otherwise. Could you please factor this out and make a case for it?

jamescheney avatar Apr 13 '25 18:04 jamescheney

Got it, I have removed this change. (Also, when the test was created in 1b72bf9d80fa57fb3712e6917c7f712d4734678d, the OTHERWISE token did not exist, but it did when the typechecking test script was added in 4d6687e076ab35cdae6d91dfb1a65e10076d041e.)

rajdakin avatar Apr 14 '25 10:04 rajdakin

The motivation of the entire PR is to make sure the tests are up-to-date, i.e. that tests which should pass do actually pass, that tests that pass are not ignored, and to fix minor issues (the case otherwise).

I think Daniel's point is that unlike fixing tests, this is a language change, and deserves to be pulled out and discussed/agreed sepatrately

Yes indeed. I think the problem is that otherwise became a keyword, when @SimonJF implemented the session exception handling syntax. We have discussed this "issue" previously. I believe the right solution is to unify our various handling-syntaxes into switch (...) { ... } (issue #57 discusses some thoughts on a better internal representation). We currently have special syntax for effect patterns. We'd need special syntax for catching session exceptions too.

dhil avatar Apr 14 '25 10:04 dhil

Ah - sorry @rajdakin, I misinterpreted your change about case otherwise as meaning that you had introduced this keyword and rather than that you were adjusting things to make ordinary case _ a desugared form of case using otherwise. It does still sound like this is something that should be discussed before further changes, though.

The issue is that session exception syntax is of the form try exp as pattern otherwise exp - it wasn't ever being used as a synonym for _ before. So even if this is a non breaking change, it is a nontrivial one. I feel it is conventional enough to write case _ or similar in functional languages, rather than have a separate keyword for catch-all or default cases; but if we do want to have such a separate keyword, for me it would be more idiomatic to write default => or (to avoid proliferation of keywords) otherwise => rather than case otherwise =>, but this does feel like bikeshedding in action, now.

jamescheney avatar Apr 14 '25 10:04 jamescheney

Is the current version good to go then? I have removed everything concerning case otherwise.

rajdakin avatar Apr 14 '25 11:04 rajdakin