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

Custom placeholders #107

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Custom placeholders #107

wants to merge 11 commits into from

Conversation

Virtlink
Copy link
Contributor

This PR adds the ability to change the syntax of placeholders in SDF3.

The configuration option would become:

placeholder {
  prefix = "$"
  suffix = ""
}

@Virtlink Virtlink requested a review from Gohla January 26, 2022 18:05
@Virtlink
Copy link
Contributor Author

Virtlink commented Feb 3, 2022

This PR is now ready for review.

Copy link
Member

@Gohla Gohla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have some feedback on exceptions and defaults. If those are addressed this can be merged.

} catch (NoSuchElementException ex) {
return CommandFeedback.of(ShowFeedback.showText("Cannot show parenthesizer; SDF3 was not configured in '" + args.root + "'", getName(args.concrete, args.root)));
} catch (Exception ex) {
return CommandFeedback.ofTryExtractMessagesFrom(ex, args.root);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching all Exceptions is bad practice, as it also catches all RuntimeExceptions indicating bugs such as NullPointerException which should just crash the build, as well as catching InterruptedExceptions which should cancel the build, etc.

Also, I don't think any code here is throwing NoSuchElementException.

AFAICS the old code was handling the exceptions as part of results correctly, so why change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoSuchElementException is thrown by Option.unwrap(). The reason I wrote it like this is because I need both the Sdf3SpecConfig and the Sdf3Config. Using the syntax as you wrote it, it would become something with deep nesting and multiple places of error handing:

return context.require(specConfigFunctionWrapper.get(), args.root).mapOrElse(
    specConfigOpt -> specConfigOpt.mapOrElse(
        specConfig -> context.require(configSupplierWrapper.get()).mapOrElse(
            configOpt -> configOpt.mapOrElse(
                config -> run(context, specConfig, config, "", args),
                () -> CommandFeedback.of(ShowFeedback.showText("Cannot show parenthesizer; SDF3 was not configured in '" + args.root + "'", getName(args.concrete, args.root)))
            ),
            // TODO: should we propagate configuration errors here? Every task that requires some configuration will
            //       propagate the same configuration errors, which would lead to duplicates.
            e -> CommandFeedback.ofTryExtractMessagesFrom(e, args.root)
        ),
        () -> CommandFeedback.of(ShowFeedback.showText("Cannot show parenthesizer; SDF3 was not configured in '" + args.root + "'", getName(args.concrete, args.root)))
    ),
    // TODO: should we propagate configuration errors here? Every task that requires some configuration will
    //       propagate the same configuration errors, which would lead to duplicates.
    e -> CommandFeedback.ofTryExtractMessagesFrom(e, args.root)
);

Or do you have a better suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If throwing/catching leads to better code, that is fine, as long as you rethrow RuntimeException and InterruptedException when you catch all Exception.

return run(context, specConfig, config, args.strategyAffix, args, name);
} catch (NoSuchElementException ex) {
return CommandFeedback.of(ShowFeedback.showText("Cannot show parse table; SDF3 was not configured in '" + args.root + "'", name));
} catch (Exception ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching all Exceptions is bad practice, as it also catches all RuntimeExceptions indicating bugs such as NullPointerException which should just crash the build, as well as catching InterruptedExceptions which should cancel the build, etc.

Also, I don't think any code here is throwing NoSuchElementException.

AFAICS the old code was handling the exceptions as part of results correctly, so why change it?

@@ -28,9 +30,14 @@ void testTask(boolean createCompletionTable) throws Exception {
textFile("src/nested/a.sdf3", "module nested/a context-free syntax A.A = <key>");
textFile("src/nested/b.sdf3", "module nested/b context-free syntax B.B = <word>");
final Sdf3SpecToParseTable taskDef = component.getSdf3SpecToParseTable();
final Sdf3SpecConfig sdf3SpecConfig = specConfig(rootDirectory.getPath());
final Sdf3Config sdf3Config = new Sdf3Config("$", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call createDefault here, so that tests always test the default?

@@ -17,9 +19,14 @@
@Test void testTask() throws Exception {
final FSResource resource = textFile("a.sdf3", "module test context-free syntax A = <A>");
final Sdf3ParseTableToParenthesizer taskDef = component.getSdf3ParseTableToParenthesizer();
final Sdf3SpecConfig sdf3SpecConfig = specConfig(rootDirectory.getPath(), rootDirectory.getPath(), resource.getPath());
final Sdf3Config sdf3Config = new Sdf3Config("$", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call createDefault here, so that tests always test the default?

@@ -13,9 +15,15 @@
class ToNormalFormTest extends TestBase {
@Test void testTask() throws Exception {
final TextResource resource = textResource("a.sdf3", "module nested/a context-free syntax A = <A>");
final Sdf3Config sdf3Config = new Sdf3Config("$", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call createDefault here, so that tests always test the default?

@@ -13,9 +15,15 @@
class ToPrettyPrinterTest extends TestBase {
@Test void testTask() throws Exception {
final TextResource resource = textResource("a.sdf3", "module nested/a context-free syntax A = <A>");
final Sdf3Config sdf3Config = new Sdf3Config("$", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call createDefault here, so that tests always test the default?

ast = doExec(context, input, strategoRuntime, ast);
return Result.ofOk(ast);
} catch(Exception e) {
return Result.ofErr(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching all Exceptions is bad practice, as it also catches all RuntimeExceptions indicating bugs such as NullPointerException which should just crash the build, as well as catching InterruptedExceptions which should cancel the build, etc.

));
return true;
} catch(RuntimeException e) {
return false; // Context not available; fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching all RuntimeExceptions is bad practice, as it catches all kinds of exceptions indicating bugs such as NullPointerException. This will silently hide them, which is very bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was based on your own code for Sdf3PpLanguageSpecNamePrimitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then that code should be changed as well. I think it should catch AdaptException instead.

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.

2 participants