Skip to content

Don't parse __autoconcat__ to sconcat in strict mode - #1225

Merged
Pieter12345 merged 1 commit into
EngineHub:masterfrom
Pieter12345:optimize_autoconcat
Nov 30, 2020
Merged

Pieter12345 merged 1 commit into
EngineHub:masterfrom
Pieter12345:optimize_autoconcat

Conversation

@Pieter12345

Copy link
Copy Markdown
Contributor

Parse __autoconcat__() to the new __statements__() instead of sconcat() in strict mode. __statements__() takes arguments of any type and returns void for typechecking, so compile errors will be generated in cases where __autoconcat__() used to insert sconcat()s, or in other words, where the user has either forgotten to put some . concat, or where the user has made a mistake.
This change does not affect non-strict mode, as automatically inserting concats is a feature there.
Alias syntax should also remain possible in strict mode, but only when the whole alias is nicely concatenated together by the user. Inserting multiple arguments/statements will cause the code block to be interpreted as a statements block and not as an alias redirect.

@Pieter12345
Pieter12345 force-pushed the optimize_autoconcat branch from 5c60821 to c6b0ced Compare July 22, 2020 02:15
@Pieter12345

Copy link
Copy Markdown
Contributor Author

This change fixes the following issues in strict mode, but does not affect them in non-strict mode:

@PseudoKnight

Copy link
Copy Markdown
Contributor

Note that #814 already has a workaround implemented for procedures. The main sconcat is executed but not returned in procedures.

@LadyCailin

Copy link
Copy Markdown
Member

The code here looks good, but let's add something about this in the documentation, since this will break traditional use of aliases if strict mode is enabled. Probably need to make some changes in the introductory documentation to ensure that the best practice (using run()) is mentioned (though we can still show examples of the standard alias syntax).

@Pieter12345

Pieter12345 commented Sep 9, 2020 •

Copy link
Copy Markdown
Contributor Author

I believe that leaves us with the following things to do/consider/realize:

  • This PR only makes changes to strict mode code.
  • This PR breaks /cmd = /cmd1 arg, and maintains support for the preferred version: *:/cmd = run('/cmd1') \ run('/cmd2') , and for the explicit concat and full string versions: /cmd = /cmd1.' arg' and /cmd = '/cmd1 arg'.
  • This PR maintains support for /cmd = /cmd1 \ run('/cmd2'), where possible arguments have to be quoted as in the first point.
  • It should be considered whether we want to allow unquoted /cmd = /cmd2 (VS /cmd = '/cmd2') commands, as this might feel weird because users would only have to quote command arguments, and handle the unquoted command itself as a string.
  • The Beginner's Guide should be updated to at least quote+concat the examples, and perhaps even only include the run() version. This is the place where users can get pushed in a certain desired direction, which might be run('/cmd arg'), but which might also be '/cmd arg'. I'd like to leave that choice with @LadyCailin. The documentation update can be pushed separately from this PR if you want to do it, as this PR only removes existing syntax, and doesn't add new syntax.

Parse `__autoconcat__()` to the new `__statements__()` instead of `sconcat()` in strict mode. `__statements__()` takes arguments of any type and returns `void` for typechecking, so compile errors will be generated in cases where `__autoconcat__()` used to insert `sconcat()`s, or in other words, where the user has either forgotten to put some `.` concat, or where the user has made a mistake.
This change does not affect non-strict mode, as automatically inserting concats is a feature there.
Alias syntax should also remain possible in strict mode, but only when the whole alias is nicely concatenated together by the user. Inserting multiple arguments/statements will cause the code block to be interpreted as a statements block and not as an alias redirect.
@Pieter12345
Pieter12345 merged commit 69f6baa into EngineHub:master Nov 30, 2020
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