Comments (5)
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
andworld
@tag "hello world
would represent a tag with a single parameter valuedhello 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.
Related Issues (20)
- Ruru rendering issues HOT 2
- GraphQL resolver emulation should cascade
- Should `__FlagStep` be isSyncAndSafe?
- Change how `__Flag` be represented in plan diagrams?
- Plan diagrams: `::sideeffectplan` and `::unbatchedPlan` conflict HOT 1
- RBAC enhancements: don't fetch column if not allowed to fetch column
- Rewrite OutputPlans, they're a mess
- `fieldArgs.getRaw` and related methods should throw if called out of turn
- Unable to pass FieldArg as parameter for resource .get() HOT 7
- Latest `grafast` depends on `graphile-config` but doesn't list dependency
- Foreign Key on Composite Type HOT 4
- PageInfo implemented incorrectly? HOT 6
- Hono adaptor HOT 2
- How to omit a class and specific field HOT 3
- pgUnionAll.single() throws an error when the list is empty HOT 2
- Grafast processFragment does not permit use of string arguments
- Use `@graphile/logger` for logging
- Improve pgSelectSingleFromRecord $record argument type
- How to Modify the Result of a PgSelectStep HOT 4
- How to get inflected database data in makeExtendSchemaPlugin? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from crystal.