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

feat: revert the spine/toc nav order check to a WARNING #1056

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

rdeltour
Copy link
Member

As agreed on the 2019-07-11 EPUB CG call:
https://www.w3.org/2019/07/11-epub3cg-minutes.html

Changes the behavior introduced in #999

Fixes #1036

As agreed on the 2019-07-11 EPUB CG call:
https://www.w3.org/2019/07/11-epub3cg-minutes.html

Changes the behavior introduced in #999

Fixes #1036
@rdeltour rdeltour added this to the 4.2.2 milestone Jul 11, 2019
@rdeltour rdeltour self-assigned this Jul 11, 2019
@mattgarrish
Copy link
Member

What about the request to explain the severity? Can we just dump something in an info message?

@mattgarrish
Copy link
Member

For example, pump out a generic suggestion like: "This rule is under review and may change in a future release. See the issue tracker."?

It just feels really awkward to output stuff like this via epubcheck, but if we make something generic then perhaps it could just be attached/detached to issues if we end up in a similar place again??

@rdeltour
Copy link
Member Author

What about the request to explain the severity? Can we just dump something in an info message?

ah right. I had started the PR before this idea was brought. I'll think about how to do this (we have "suggestion" messages, that take another line and are bound to a message, and that might be usable for that purpose).

@rdeltour
Copy link
Member Author

For example, pump out a generic suggestion like: "This rule is under review and may change in a future release. See the issue tracker."?

We could add that to a new line, but then that might broke any tool that rely on regex parsing? (other messages consistently start with INFO, USAGE, WARNING, or ERROR…)

Or we can append that to the message text, but then the line would get quite long…

@mattgarrish
Copy link
Member

We could add that to a new line, but then that might broke any tool that rely on regex parsing?

I have to admit to never turning on any verbosity, so I just assumed such things could come out with their error/warning but labeled as info or some such on a new line.

I have no problem putting a prefix in front of the message, if that's the issue. I just had no idea what it should be so didn't add to the suggested text.

@rdeltour
Copy link
Member Author

I ended up adding a new INFO message, which has the benefit of not needing to update the current messaging mechanism and is output by default.

The message will read:

WARNING(NAV-011): ./src/test/resources/30/expanded/invalid/nav-page-list-unordered-spine.epub/OPS/nav.xhtml(20,40): 'page-list' nav must be in reading order; link target 'OPS/content_002.xhtml#p2' is before the previous link's target in spine order.
INFO(INF-001): ./src/test/resources/30/expanded/invalid/nav-page-list-unordered-spine.epub/OPS/nav.xhtml(20,40): The previous rule is under review and may change to an 'ERROR' in a future release. See the discussion at https://github.com/w3c/publ-epub-revision/issues/1283

@bduga can you please confirm this is OK with what you suggested in the CG call?

@bduga
Copy link

bduga commented Jul 15, 2019

Looks good to me!

@rdeltour
Copy link
Member Author

@mattgarrish since your review is still marked as pending, can you please confirm you're OK too with the info message?

@rdeltour rdeltour merged commit 1f6a882 into master Jul 17, 2019
@shiestyle
Copy link

Let me post a kind of concern.

‘ERROR’ in the INFO message may not comfortable if the system using EPUBCheck extract ‘ERROR’ as text by regular expression from the result message.

@rdeltour
Copy link
Member Author

rdeltour commented Jul 17, 2019

‘ERROR’ in the INFO message may not comfortable if the system using EPUBCheck extract ‘ERROR’ as text by regular expression from the result message.

I hadn't thought of that (and just merged the PR). The change did intend to not break existing regex-based output parsers, notably by relying on the existing message mechanism rather than just adding a new line. I would expect an output parser to use a regex checking that ERROR only happens at the beginning of the line…

Is your concern grounded on an existing system?

@shiestyle
Copy link

Yes, if existing system will use /^ERROR/ or /ERROR(.*):/ for extraction, it's no problem but just use of /ERROR/ should be modified.

The objective to change ERROR to WARNING is to avoid error detection by eBook stores when accepting EPUBs, and then NOT using 'ERROR' word in the result message will be safer, I think.

Anyway, sorry for my too late reaction.

@rdeltour rdeltour deleted the fix/1036/nav-toc-order branch July 18, 2019 08:28
rdeltour added a commit that referenced this pull request Jul 18, 2019
This addresses the concern raised by @ShinyaTakami in #1056.
rdeltour added a commit that referenced this pull request Jul 18, 2019
This addresses the concern raised by @ShinyaTakami in #1056.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sudden errors by different order of toc/spine by EPUBCheck v4.2.0-rc
4 participants