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

Re-implement vocabulary association mechanisms as an algorithm #2379

Merged
merged 14 commits into from
Aug 7, 2022

Conversation

mattgarrish
Copy link
Member

@mattgarrish mattgarrish commented Jul 28, 2022

Not looking for approval at this time. Just curious for early feedback on whether this makes sense as a replacement.

I kept the one requirement not to blow up when encountering unknown prefixes.

Fixes #2378
Fixes #2382


Preview | Diff

@iherman
Copy link
Member

iherman commented Jul 28, 2022

I think that is great. It would get around the issue of the awkward MUST statements and it works well.

Thanks!

…ection;

add optional expansion of epub:type values;
rename section to "Processing property values;
clarify that reading systems are not required to expand values and may use the values as strings for processing behaviours
@mattgarrish
Copy link
Member Author

Okay, I think this is good to go now. The last commit mostly added explanations for all the steps. I also made a few tweaks to infra-ize things a bit more.

The two changes of note are:

  • I've added an optional item to the epub:type processing list to get an expanded url. It was a bit strange to describe processing the attribute without mentioning that you could get an expanded value to process.
  • I also added a paragraph to the top of the algorithm clarifying that it's not required to expand property values to use them. Reading systems that don't support the vocabulary association mechanisms can just treat them like strings.

@mattgarrish
Copy link
Member Author

And as reading the diff is a bit of a nightmare, here's a link to the current section if you want to manually compare against the preview: https://www.w3.org/TR/epub-rs-33/#sec-vocab-assoc

epub33/rs/index.html Show resolved Hide resolved
epub33/rs/index.html Outdated Show resolved Hide resolved
epub33/rs/index.html Outdated Show resolved Hide resolved
epub33/rs/index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented Jul 29, 2022

I have sketched out an implementation in typescript, just to check. Emphasis is on "sketch", but it helped me to follow the infra stuff:-)

@iherman
Copy link
Member

iherman commented Aug 5, 2022

The issue was discussed in a meeting on 2022-08-04

List of resolutions:

View the transcript

2. Re-implement vocabulary association mechanisms as an algorithm (pr epub-specs#2379)

See github pull request epub-specs#2379.

See github issue epub-specs#2378.

See github issue epub-specs#2382.

Dave Cramer: there are a couple issues associated with this (#2378, #2382).
… this is very technical, I wish mgarrish were here to explain.
… doesn't really seems to change how the spec works, more an editorial change.
… seems like they were just looking for WG approval before merging, even though most of us probably won't have strong opinions on this.
… i'm generally in favor of explaining things in an algorithmic fashion rather than hard to understand paragraphs.
… maybe we just say that we don't have objection to merging this pending github reviews?.

Proposed resolution: The WG supports merging #2379 pending approval from the assigned reviewers.. (Dave Cramer)

Dave Cramer: +1.

Matthew Chan: +1.

Brady Duga: +1.

Toshiaki Koike: +1.

Shinya Takami (高見真也): +1.

Resolution #1: The WG supports merging #2379 pending approval from the assigned reviewers..

@iherman
Copy link
Member

iherman commented Aug 5, 2022

Per WG resolution, we need an approval of at least @shiestyle and @dauwhe (@wareid and, I presume, @rdeltour being on vacations...). Shinya, Dave, could you do this, please?

@mattgarrish
Copy link
Member Author

I think they gave us the green light to merge it once we are happy with it. I think the "reviewers" in this case means me and you, as it sound like the group is fine with whatever we resolve on for this given no one's hugely passionate about a feature that's only for internal reading system use... 😄

@iherman iherman merged commit 0e0ac4a into main Aug 7, 2022
@iherman iherman deleted the experiment/issue-2378 branch August 7, 2022 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants