Trailing space after `@primaryKe <name>y` smart tag causes error when used with `|` to modify virtual constraint about crystal HOT 5 CLOSED

Yes, building a smart tags system has been a desired goal for a long time, but it’s super low priority compared to everything else. I’m sure we’ll get round to it one day. I’m having to be pretty ruthless on which things I work on otherwise we’ll never release V5; there’s always V6 for stuff like this.

from crystal.

The spec does not allow for trailing whitespace; it’s a pretty strict spec: https://postgraphile.org/postgraphile/next/smart-comments#smart-comment-spec

Trailing space can be significant because @tag is boolean true, but @tag (with trailing space) is a string tag where the string happens to be empty. Changing this is acceptable in V5 since we’re still in beta; but I am not particularly bothered about it, feel free to take this on if you really want to (and raise a PR), but I’m going to close this as I’m fine with the current behaviour.

from crystal.

As a user I find this behavior very strange and I would expect you to receive more bug reports with similar topics in the future.

I do not believe the spec, as currently written, is sufficiently clear that trailing whitespace is interpreted as belonging to the tag's parameters. Even if it was, I still think users might end up spending time debugging trailing spaces as not all editors highlight these.

Personally, I think that any space that should be part of the tag's options should require quoting. This also includes the empty string imo.

I.e.

• @tag and @tag should represent an equivalent boolean tag
• @tag "" would represent a string tag with an empty string
• @tag hello world would represent a tag with two parameters, hello and world
• @tag "hello world would represent a tag with a single parameter valued hello world

from crystal.

Well it’s been 5 years or so… not many people have mentioned it 🤷‍♂️ Changing such a well established feature to have such different semantics would be quite a significant change to make, so it would have to be done during the beta; but V5 has been in development now for 4 years and I have to draw the line somewhere, this latest proposed change is non-essential and is non-trivial (would involve changing documentation and probably test cases too) so I will not be implementing it. If you want to take it on have at it, but please make sure you also address all the third party plugins that rely on smart tag syntax too. One option is that you introduce an alternative symbol instead of @ with this new syntax so that the change can be made in a backwards compatible way.

Your new syntax with multiple parameters might not be the best structure in all cases, for example the @foreignKey smart tag, if the user added spaces around parenthesis, would recognise an open bracket as a parameter which would be super weird. I think we’d just join the separate parameters back together with spaces though so no big deal. Or i guess we could completely change the semantics to be less SQL-syntax-like and more CLI-arguments-like. I leave that for you to investigate.

If you want to target a smaller change you could make it so the pipe symbol ignores (absorbs) surrounding whitespace. This would only affect the smart tags that support this feature (since it is not a core feature of smart tags and instead something each tag may manually implement, so would need fixing on a case by case basis) so is a much smaller and more controlled change, and we know for @foreignkey for example that it shouldn’t break anything so is a safe change to make.

from crystal.

Your new syntax with multiple parameters

I might not have read up sufficiently on plugins to know how arguments are passed but I'm not looking to introduce a new syntax for multiple parameters.

Put more simply, I would like to .trim() the argument of any smart tag and require quoting of identifiers containing whitespace (including the empty string). I don't think this should be a very breaking change (though it might still be better to introduce during the beta) except for certain edge cases where plugins rely on syntax I find to be problematic (such as @foo ).

If you want to target a smaller change you could make it so the pipe symbol ignores (absorbs) surrounding whitespace.

This seems like a reasonable change. Though, without quoting, you are actually able to break tags under specific circumstances. Consider @foo vs @foo | @bar. Is @foo now a boolean tag or an empty string tag?

I probably won't have time to create a PR for quoting or any solution beyond a quick addition to the docs; I have a long list of todos right now and only so much spare time to do it.

One such addition to the docs could be to add a sentence clearly starting that trailing whitespace will be interpreted as part of the tag's argument. Though I disagree with this syntax, that would make the spec sufficiently explicit regarding this behaviour.

Or i guess we could completely change the semantics to be less SQL-syntax-like and more CLI-arguments-like.

I'm not sure that's the right way to go (especially this late in the development process) but an interesting side effect of that is that you could rely on third party argument parsing libraries, reducing some of the maintenance load 🤔

A third party argument parser might even be able to provide validation warnings for improper tag use. That could be really helpful for end-users.

from crystal.