search

MarcusBarnes / mik

The Move to Islandora Kit is an extensible PHP command-line tool for converting source content and metadata into packages suitable for importing into Islandora (or other digital repository and preservations systems).
GNU General Public License v3.0
34 stars 11 forks source link

Issue 370 #386

Closed mjordan closed 7 years ago

mjordan commented 7 years ago

Github issue: (#370)

What does this Pull Request do?

Provides a configuration option for the CSV books and newspapers toolchains that allows the use of a page-sequence delimiter other than the currently hard-coded -.

What's new?

This PR adds a new configuration option used by the CSV books and newspapers writers to define the page sequence delimiter, defaulting to a hyphen (-), so this change should not be backwards-breaking. If a user wants to use another. The delimter is defined by the user by adding the following setting in the [WRITER] section of the .ini file:

page_sequence_separator = _

for example.

How should this be tested?

Following the new "testable" workflow originally described in #370 and under discussion in #384:

  1. Check out the issue-370 branch.
  2. Run phpunit --exclude-group inputvalidators --bootstrap vendor/autoload.php tests.
  3. All tests should pass.

The PHPUnit tests add a non-default value to the XCSVBooks test suite's testWritePackages() test, and provides assets that are also used in that test. The default case (where the delimeter uses the default of -) is coverd by the XCsvNewspapers tests (which still pass) are unaltered in this PR.

mjordan commented 7 years ago

I forgot to add those assets for the new test.... just pushed them now.

MarcusBarnes commented 7 years ago

To confirm, when I ran the tests I got the successful result: OK (46 tests, 65 assertions). Are the number of tests and assertions the same for you?

mjordan commented 7 years ago

Yes, that's what I get too. In fact, we probably should indicate how many tests and assertions we expect the reviewer to get.

MarcusBarnes commented 7 years ago

Agreed. Other than documenting that we should mention the number of expected successful tests and assertions, we should update the pull-request template appropriately to help us remember to do so.

mjordan commented 7 years ago

OK, I'll modify the proposed workflow described in #384, and also open a PR for the new PR template (woah, meta).