daisy / pipeline

Super-project that aggregates all Pipeline related code, provides a common tracker for Pipeline related issues and holds the Pipeline website
http://daisy.github.io/pipeline
21 stars 21 forks source link

Portage of pipeline 1 dtbook cleaning routines #686

Closed NPavie closed 8 months ago

NPavie commented 1 year ago

Reintagrating pipeline 1 dtbook cleaning routines in pipeline 2.

The routines logic is stored in dtbook-utils modules while it is exposed as a pipeline script with a new dtbook-cleaner script modules.

NPavie commented 1 year ago

@bertfrees sorry just saw your comment.

I don't think i'll have time to add the tests before the next release. If you want to merge the PR for the upcoming release, i can do a new PR later to include them (but i need some time to prepare them).

bertfrees commented 1 year ago

I've squashed your 4 commits, added a cleanup commit, and rebased onto the latest master.

bertfrees commented 1 year ago

@NPavie Reminder that this PR needs some tests before it can be merged.

NPavie commented 1 year ago

@bertfrees i finished a last round in tests on daisy #718 , i have a small update to make on SaveAsDAISY and i'll resume the creation of tests for this PR.

NPavie commented 1 year ago

Hi @bertfrees, i added a small set of tests for the cleaning routines and a bugfix in an xsl that i detected while doing the tests.

(I did a rebase over the latest state of master before force pushing the changes)

bertfrees commented 11 months ago

@NPavie Thanks for the fixes! Looking much better now. The only issues left are the following:

NPavie commented 10 months ago

@bert i fixed the failing test (forgot to change a title)

I also removed the mention to pipeline 1 narrator in the current documentation, to specify that it's more of a cleanup of the dtbook for audio-synthesis.

IMO, the documentation might be of use for users who would want to know more precisely what the script is doing while cleaning. A separate doc folder/markdown file, with links to it in the script inner documentation, makes sense. If its ok with you, i'll move the detailed script documentation to a doc/index.md following the dtbook-to-zedai model you mentionned.

Just in case i forget (but I might make a separate issue for it), another test is failing on windows for this module :

Failed tests:
  test_load
    if the base uri starts with file:///
      * FAILURE: this results in a xml:base with only one slash after file:

On my machine, the resulting fileset as xml:base="file:/F:/" (F: is the disk my terminal is currently positionned on when running the tests) As the "/" path does not really exists on Windows, it seems like the produced result is correct (but on windows case, the result itself will depends on dev system configuration and from which disk the test is run) I don't know if you can disable tests on a system basis with XProcSpec (like excluding a system for specific tests)

bertfrees commented 10 months ago

@NPavie Thanks! Yes, moving the detailed script documentation to a doc/index.md is perfect.

Regarding the other failing test: yes, please create a new issue. That's something we want to fix but not in the upcoming release. You can not disable an XProcSpec scenarion based on which platform the tests are running on. If you want to do that you need to put it in a separate XProcSpec file and then mess with src/test/java/XProcSpecTest.java (override the runXProcSpec method of AbstractXSpecAndXProcSpecTest). But we should avoid that if possible. It's probably better to change the test. For now you could add a pending attribute.

bertfrees commented 8 months ago

Merged in ed88343