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

adding ABNF section #24

Merged
merged 7 commits into from Mar 24, 2020
Merged

adding ABNF section #24

merged 7 commits into from Mar 24, 2020

Conversation

@jscancella
Copy link
Contributor

jscancella commented Dec 16, 2019

This Pull Request integrates #23 into the index.html.

@jscancella jscancella requested a review from ruebot Dec 16, 2019
@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Dec 16, 2019

@jscancella can you provide some rationale and background for this PR so that folks reviewing and providing feedback can be better informed?

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Dec 16, 2019

@ruebot sure!
ABNF is often used to auto generate code by many projects. This would those projects to automatically create code/software that integrates with the specification. Additionally, many RFC specifications like to have ABNF as it defines in a very concrete way a specification.

I think it is also just another way to communicate the specification and make sure there is no ambiguity in the specification that can cause problems later (like what happened in the BagIt specification).

@ruebot ruebot requested a review from tdilauro Dec 16, 2019
@tdilauro tdilauro mentioned this pull request Dec 18, 2019
index.html Outdated
profile-description = DQUOTE "External-Description" DQUOTE ":" value ","
profile-version = DQUOTE "Version" DQUOTE ":" version ","
version = DQUOTE 1*DIGIT "." 1*DIGIT DQUOTE
bag-info = DQUOTE "Bagit-Info" DQUOTE ":{" info-organization info-address info-name info-phone info-email info-description info-identifier info-size info-group-identifier info-count info-sender-identifier info-sender-description info-date info-payload-oxum "},"

This comment has been minimized.

Copy link
@tdilauro

tdilauro Dec 18, 2019

Member

The BagIt Profiles spec does not limit the tags that may be specified in the Bag-Info section, specifically noting support for arbitrary tags, including tags not enumerated in the BagIt spec.

index.html Outdated
email-address = DQUOTE addr-spec DQUOTE
profile-description = DQUOTE "External-Description" DQUOTE ":" value ","
profile-version = DQUOTE "Version" DQUOTE ":" version ","
version = DQUOTE 1*DIGIT "." 1*DIGIT DQUOTE

This comment has been minimized.

Copy link
@tdilauro

tdilauro Dec 18, 2019

Member

version rule supports multiple other rules with varying numbers of dots in their version numbers. A more generalized version production would be. Might considering calling it "dotted-version" to allow for more generalized (undotted) version strings in the future.

version = DQUOTE 1*DIGIT *("." 1*DIGIT) DQUOTE
  or
dotted-version = DQUOTE 1*DIGIT *("." 1*DIGIT) DQUOTE
@jscancella jscancella requested a review from tdilauro Dec 18, 2019
Copy link
Member

tdilauro left a comment

@jscancella I have some general concerns about

  • the maintainability / grokability of the ABNF in its current form,
    • e.g., bag-info and related rules
  • correctness
    • e.g., assumptions about property ordering
  • precision
    • e.g., VCHAR includes the quote character (DQUOTE), which cannot appear in JSON values without being escaped
    • e.g., case sensitive matches (use %s notation RFC 7405)

I’m away on vacation through the end of the year, but will try to find a chunk of time before the new year to dig deep enough to make these concerns a little less vague.

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Dec 18, 2019

@tdilauro I will also be gone most of January and unable to work on this until I get back. Hopefully I can finish it before I leave.

  • Maintainability/GROK - if you have suggestions I am open to looking at them and incorporating them into the pull request. Also see 701e922 and a42d0f4 which I think make it look better.
  • correctness - I followed the order that was given in the normative section. I didn't see it say anywhere that there was a specific order they had to be in, and when parsing JSON you shouldn't need a particular order
  • precision
    • VCHAR - unless you really want to get ugly ABNF rules, I don't see how you are going to specify that you have to escape the quotes in values.

    • case sensitive - good to know it is by default case INSENSITIVE. However, do we really want to force case senstivity for what are basically map keys? @ruebot, what are your thoughts on this?

jscancella added 2 commits Dec 18, 2019
@tdilauro

This comment has been minimized.

Copy link
Member

tdilauro commented Dec 18, 2019

@jscancella Ugh. I just wanted to get my vague concerns out there, but didn't mean to cause any undo stress. I'll try to have a look sooner rather than later, but have to balance family vacation time. If it doesn't work out, we can always iterate later.

@tdilauro I will also be gone most of January and unable to work on this until I get back. Hopefully I can finish it before I leave.

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Dec 18, 2019

@jscancella Ugh. I just wanted to get my vague concerns out there, but didn't mean to cause any undo stress. I'll try to have a look sooner rather than later, but have to balance family vacation time. If it doesn't work out, we can always iterate later.

Don't worry about it, family is more important especially this time of year. Look at it when you get a chance, but don't stress.

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Dec 19, 2019

case sensitivity

BagIt touches on the challenges here. Basically says it depends on the file system. But, JSON is case sensitive. Since a BagIt Profile is JSON, then a BagIt Profile is case sensitive, right?

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Dec 19, 2019

Maybe it didn't make it into the specification, but I remember talking about https://tools.ietf.org/html/rfc8493#section-6.1.1.1 with Chris Adams and how we should try and preserve case, but not force anyone to be case sensitive when looking up information.
For instance, if we have a bag object and want to get a particular key in the bag-info we shouldn't be case sensitive. But when we go to write the bag-info (or other parts of the bag) we should preserve the case sensitivity entered and write it exactly the same out to file.

I looked at https://tools.ietf.org/html/rfc7159 but couldn't find where it says that strings are case sensitive. Only that they are required to test for equality (see section 8.3).

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Dec 19, 2019

Also I just found in ECMA-404

An object structure is represented as a pair of curly bracket tokens surrounding zero or more name/value pairs. A name is a string. A single colon token follows each name, separating the name from the value. A single comma token separates a value from a following name. The JSON syntax does not impose any restrictions on the strings used as names, does not require that name strings be unique, and does not assign any significance to the ordering of name/value pairs. These are all semantic considerations that may be defined by JSON processors or in specifications defining specific uses of JSON for data interchange.

Perhaps I am not understanding, but from the wording above it seems it is up to the individual parsers to determine if the name is case sensitive.

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Jan 22, 2020

How about we just go with insensitive? It'd be consistent with what @acdha and your discussion points here, and we could punt to a future BagIt update. Though, it's kinda tough since we're trying a thread a needle between BagIt and JSON specs.

@tdilauro

This comment has been minimized.

Copy link
Member

tdilauro commented Jan 22, 2020

I think that's fine, but we should be explicit about it. Additionally, the current release of the validator and my recently committed PR are case sensitive.

index.html Outdated
@@ -58,7 +58,7 @@ <h1>Introduction</h1>
<p>
<pre>

BagIt Profile JSON file

This comment has been minimized.

Copy link
@jscancella

jscancella Jan 23, 2020

Author Contributor

this is from merging latest from master

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Feb 3, 2020

@jscancella @tdilauro do we need to create a separate issue for the case sensitivity, or should we just address it in this PR? Once we sort that out, we can create an issue for the validator.

@jscancella

This comment has been minimized.

Copy link
Contributor Author

jscancella commented Feb 4, 2020

Unless I am misunderstanding I think it is resolved - make it case insensitive. I haven't seen any proposed updates on the forum for this either, so should we go ahead and merge it?

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Feb 4, 2020

@jscancella if you resolve the conflict, I'm happy to hit approve if everything looks good. Then you're welcome to merge and write your own commit message.

@tdilauro

This comment has been minimized.

Copy link
Member

tdilauro commented Feb 4, 2020

The latest validator is case sensitive. Shouldn't these be in sync?

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Feb 4, 2020

@tdilauro yeah, ideally they should be in sync. If either you have time, feel free to go for it. Otherwise, it might be a bit before I can get to it. I'll create an issue over on the validator repo.

@ruebot
ruebot approved these changes Feb 4, 2020
@ruebot ruebot merged commit 15eb5ac into bagit-profiles:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.