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

Ignore unknown metadata fields #10

Closed
wants to merge 1 commit into from

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Oct 6, 2018

For forwards-compatibility reasons, request that programs ignore unknown metadata fields. The alternative is for old programs to completely ignore new reviews… that's possible too, I don't have any strong opinions. Will make things harder for #6, though.

@Ekleog Ekleog requested a review from oxij October 6, 2018 19:44
@Ekleog Ekleog mentioned this pull request Oct 7, 2018
@oxij
Copy link
Member

oxij commented Oct 8, 2018 via email

This was referenced Oct 8, 2018
@Ekleog
Copy link
Contributor Author

Ekleog commented Oct 9, 2018

Good catch about the x- identifier, thanks! I've pushed a new version, in addition this wording should solve #6.

@Ekleog
Copy link
Contributor Author

Ekleog commented Oct 12, 2018

@oxij Ping review :)

@oxij
Copy link
Member

oxij commented Oct 12, 2018

I would remove " or processed in an implementation-dependent way". It is IMHO obvious that you can process them with "MAY ignore" without that phrase, but the whole sentence gets confusing with "MAY ignore or ". I feel like it needs parentheses there

Unknown metadata fields with names starting with x- MAY be {ignored or processed in an implementation-dependent way}, while other unknown metadata fields SHOULD trigger a warning and MUST cause the review to be ignored.

to show what that "or" binds. Which would be possible to express if this was written in lojban, but not in English.

@Ekleog
Copy link
Contributor Author

Ekleog commented Oct 12, 2018

Hmm, I fear that without the “or”, it could be interpreted as “MAY either be ignored or MUST cause the review to be ignored”. I've pushed a rewording with either/or balancing to make the parenthesis clear, hopefully in plain english too :)

@oxij
Copy link
Member

oxij commented Oct 12, 2018

See #4 (comment).

@oxij
Copy link
Member

oxij commented Oct 12, 2018

Current phrasing LGTM, btw.

@Ekleog
Copy link
Contributor Author

Ekleog commented Oct 13, 2018

IMO x- headers avoid the need for standardizing content-type: tools could just set x-git-appraise yes. This would help making the spec as minimal as possible, while staying open to extension.

@Ekleog
Copy link
Contributor Author

Ekleog commented Oct 25, 2018

This is blocked on #4

@Ekleog Ekleog added the blocked Blocked on another issue label Oct 25, 2018
@Ekleog
Copy link
Contributor Author

Ekleog commented Jul 11, 2022

Closing as the git-wotr idea is pretty much dead and I'd like to clean up my "open PR" list so it only has actionable stuff

@Ekleog Ekleog closed this Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants