Skip to content

Conversation

rajdakin
Copy link
Contributor

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.

@dhil
Copy link
Member

dhil commented Apr 12, 2025

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;

@rajdakin
Copy link
Contributor Author

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).

@jamescheney
Copy link
Contributor

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?

@rajdakin
Copy link
Contributor Author

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

@dhil
Copy link
Member

dhil commented Apr 14, 2025

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.

@jamescheney
Copy link
Contributor

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.

@rajdakin
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants