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.

Sign up for GitHub

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

Jump to bottom

fix: report ZIP checks after the 'Validating…' message #1027

Merged
merged 1 commit into from
Mar 29, 2019
Merged

fix: report ZIP checks after the 'Validating…' message #1027

merged 1 commit into from
Mar 29, 2019

Conversation

rdeltour
Copy link
Member

Internal changes:

  • move the zip header checks from the EpubCheck class to the
    OCFChecker class
  • run the checks after the "Validating using EPUB version…" message
  • make sure the checks are run in any case even when the validation
    aborts before the "Validating using EPUB version…" message.

Test changes:

  • add a CLI test case for the message ordering
  • update the invalid mimetype test files to be based on the minimal EPUB
  • remove a duplicate test case (lorem-mimetype-2)

Fix #1025

@rdeltour
fix: report ZIP checks after the 'Validating…' message
73b0ee8 
Internal changes:

- move the zip header checks from the `EpubCheck` class to the
  `OCFChecker` class
- run the checks _after_ the "Validating using EPUB version…" message
- make sure the checks are run in any case even when the validation
  aborts before the "Validating using EPUB version…" message.

Test changes:

- add a CLI test case for the message ordering
- update the invalid mimetype test files to be based on the minimal EPUB
- remove a duplicate test case (`lorem-mimetype-2`)

Fix #1025
@rdeltour rdeltour added this to the 4.2.0 milestone Mar 27, 2019
@rdeltour rdeltour self-assigned this Mar 27, 2019
@rdeltour rdeltour requested a review from mattgarrish March 27, 2019 11:15
mattgarrish
mattgarrish approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@mattgarrish mattgarrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit unwieldy to have to know that checkzipheader needs to be run before any return, but can leave that kind of refactoring for another day.

@mattgarrish
Copy link
Member

mattgarrish commented Mar 27, 2019 •
edited
Loading

Slightly unrelated, but tied to the spec issue @dauwhe reported in w3c/epub-specs#1250 is that epubcheck barfs if the mimetype has a newline at the end, but oddly is silent if you just whitespace pad the end of the media type. We should check no leading or trailing whitespace of any kind. I'll open a new issue for this.

@rdeltour
Copy link
Member Author

Seems a bit unwieldy to have to know that checkzipheader needs to be run before any return, but can leave that kind of refactoring for another day.

Yeah I agree. This was the most straightforward way to both reorder the messages and make sure there was no behavior regression; this is definitely worth refactoring later, eg when we rework the reporting API.

@rdeltour
Copy link
Member Author

We should check no leading or trailing whitespace of any kind.

OK, I’ll add a commit to this PR.

@rdeltour rdeltour merged commit 73b0ee8 into master Mar 29, 2019
@rdeltour rdeltour deleted the fix/zip-checks-msg-order branch March 29, 2019 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattgarrish mattgarrish mattgarrish approved these changes

Assignees

@rdeltour rdeltour

Labels
None yet
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rdeltour @mattgarrish