-
Notifications
You must be signed in to change notification settings - Fork 105
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
Refine definition of extra metadata tags #79
base: master
Are you sure you want to change the base?
Conversation
Fix potential ambiguities or issues with extra metadata: 1. original rules disallowed colons in `value`. This is unnecessarily restrictive: everything is whitespace separated, so allowing colons in the value of extra metadata does not introduce any ambiguity in parsing. This allows extra metadata like `key:val:ue` to be correctly parsed as the key `key` and value `val:ue`. 2. Make it clear than the value may be empty. This was previously not defined. Allowing an empty value is also syntactically unambiguous, so this should be fine. This allows an extra metadata entry like `key:`, where the key is `key` and the value is an empty string. 3. Make it clear that the key must not be empty. This was previously not defined. It does not make sense for there to be an extra metadata where the key is an empty string. As such, entries such as `:` or `:something` must not be parsed as extra metadata tags.
8cf9ebe
to
2147e20
Compare
@swalladge nice PR, really great description in the PR for the "why" and implications/examples.
👍 I like it, this explanation would be a good addition to the readme
I'm less keen on this, consider an entry such as:
This would then end up with a key of "Wright" which I would argue is not useful. Colons are used in English in this form so this could leave to a number of these empty ones (we already have the issue with URLs but that's already part of the spec)
Agreed, again I think what you wrote for the PR is a good additional explanation to add to the readme :-) Also see #77 which I think is relevant. I'm hoping we get more reviewer comments on this as all of the above is my interpretation. Thanks for taking the time to think about this and contribute. |
Thanks for the feedback @clach04 ! Re the empty value part - what is the current assumption, considering that it wasn't previously defined? (For what it's worth, I'm not fussed either way, as long is it's clearly defined. In my workflows, I've used an empty value as a 'null', but it could just as easily be a literal 'due:null' for example.)
So for these examples (including #77), I would argue that this is a separate issue to the syntax definition. This is more of a language or domain specific data issue, which I don't think we can or should fix through syntax rules. Many things have embedded colons, with or without whitespace: times (16:35), mac addresses (4d:34:c2:...), sentences (example: this is an example), urls (ftp://server, http://..., https://..., file://..., file:name), colon separated data (column1:column2:column3), brand names, etc. What I would propose instead (and how I'm planning to handle it in an unreleased tool I'm writing), is:
|
Your suggestions are all making sense to me, @swalladge, with the addition of your last comment suggesting its for implementors to define actions on fields of interest. This also copes with the (English text) case of |
@swalladge Re the empty value part you've convinced me :-) the key values really is application specific 👍 #8 has some real ones seen in the wild. When you are ready, please share your tool, I'd be interested in taking a look :-) |
Thanks @jgclark and @clach04 ! So it looks like I need to update the readme with some of the examples, and maybe the rational for how extra metadata could be implemented in tools to avoid the issues discussed above. Would you have any other concrete feedback or requested changes before you might consider merging?
Absolutely! I fully intend to open source it - it's just right now it's still in early stages and I haven't had much time recently to dedicate to development. :) |
Nothing beyond what I commented on, the PR explanation you gave was IMHO excellent and should be included in the readme as extra context. RE merging, none of the people involved in this discussion so far have commit/merge permissions for this repo, see the open PRs (e.g. the one preceding yours). But I would merge into mine ;-) |
Fix potential ambiguities or issues with extra metadata:
value
.This is unnecessarily restrictive:
everything is whitespace separated,
so allowing colons in the value of extra metadata does not introduce
any ambiguity in parsing.
This allows extra metadata like
key:val:ue
to becorrectly parsed as the key
key
and valueval:ue
.This was previously not defined.
Allowing an empty value is also syntactically unambiguous,
so this should be fine.
This allows an extra metadata entry like
key:
,where the key is
key
and the value is an empty string.This was previously not defined.
It does not make sense for there to be an extra metadata
where the key is an empty string.
As such, entries such as
:
or:something
must not be parsed as extra metadata tags (but rather as part of the description text).