Rename conflicting --option in islandora_newspaper_batch_preprocess #9
Conversation
|
Thanks for this, @JacobSanford ! In an effort to keep our licensing up to date, we ask that all contributions to the Islandora codebase be done under a Contributor License Agreement. Could you complete either an individual CLA (http://islandora.ca/sites/default/files/islandora_cla.pdf) or if this is done on behalf of your employer, get them to complete a Corporate CLA (http://islandora.ca/sites/default/files/islandora_ccla.pdf), and send them along to me? There's more info at contributing.md, if you're curious https://github.com/Islandora/islandora_newspaper_batch/blob/7.x/CONTRIBUTING.md |
|
We should document this in a JIRA ticket too. |
|
@JacobSanford can you open a JIRA ticket like @ruebot says? It'll take a me a little while to get set up to test this but I should get to it today. |
|
The README.md will also need to be updated to reflect the new option (--data_source). Also, Islandora Batch and Book Batch use the --target option, so they'll need to have the same change applied. |
|
Doesn't make sense that travis would fail on |
|
If a single job from a build fails, you just restart that job (not the entire build). Usually it is just a TravisCI hiccup. |
|
...which I have done here. |
|
Thanks @ruebot! @JacobSanford I've tested this PR using drush 7.10 (both using a directory and a .zip file as the data source) and encountered no problems. However, I can't find any info on the |
|
@JacobSanford bump |
|
Sorry for coming in late but I don't understand how this conflicts, each drush command has its own options. This could also affect islandora_batch and islandora_book_batch @JacobSanford could you provide the drush command you run that causes the error? |
|
Hi Everyone, I haven't/don't have much time to devote to this, but adding the original target option appears to alter the database target. I am also making assumptions regarding the source of the error, but adding --target seems to alter the database connection, and I know --target is a drush option. IMHO avoiding conflicting namespaces is positive even with no obvious repercussions. I just quickly ran this: https://gist.github.com/JacobSanford/12612d42080cce324b4b Hopefully that is enough to run with. If you need more, do let me know and I'll try to get it to you ASAP. JS |
|
@JacobSanford can you create the JIRA ticket? https://github.com/Islandora/islandora_newspaper_batch/blob/7.x/CONTRIBUTING.md |
|
@JacobSanford any update on the CCLA as well? We'll need that before we can merge anything. |
|
...and @JacobSanford you'll need to update the pull request with the review by @mjordan, and in order the resolve the ticket, we'll need to address the other modules that use |
|
Ok I stand corrected, @JacobSanford is completely correct that this has become an issue in drush 7. I have opened an issue with drush to see if this is expected behaviour as I couldn't locate any notice or warning about option name conflicts. But we can rename the options for our commands as well. |
|
@JacobSanford Today those present at the committers call voted to change all batch modules to use 'islandora_target' instead of 'data_source'. Would you mind updating your PR? I'll then merge it. |
|
@mjordan (Cross-posted from JIRA issue https://jira.duraspace.org/browse/ISLANDORA-1579) I'd be happy to change this when I get some time. I wonder, however, now that the arguments are being changed if it is worth re-considering the use of 'target' in a batch to define the data source. IMHO this is contrary to (at least my) user expectation. Something like 'data_source' seems more appropriate. 'source_files'? |
|
@JacobSanford I totally agree that 'source_files' is more descriptive. In fact, during the committers call I suggested that (or perhaps it was 'source_directory') as a replacement for 'target' but since multiple modules will need to be updated, the decision was put to a vote and the tyranny of the thumbs ruled that 'islandora_target' was preferable. |
|
@bricas @JacobSanford you're more than welcome to attend Committers Calls on Thursdays and voice your opinions. |
|
@JacobSanford I'll be on this week's committers call, and more than happy to ask if folks are willing to revisit it if that would help. For what it's worth, I agree with you on the naming. |
|
@JacobSanford do you have an estimate on when you'll be able to finish the work on this ticket? It would be nice to get this done before the code freeze next week so you won't have to have pull requests against the dev branch and release branch. And, it would be nice to have this in the release and resolved 😄 |
|
@JacobSanford bump |
|
@manez Should we talk about this on the next committers calls? How to handle partially finished pull requests that go stale communication-wise? Maybe create a policy on closing them? |
|
@ruebot Unfortunately I cannot commit to fixing the expanded scope in any timely manner. We have a corrective patch being applied locally when we build, so having this fixed upstream isn't urgent for us. I understand the desire to fix this systemically, so if you do want to leave this open we can try to fit it in at a later time. If not, no hard feelings here. |
|
@JacobSanford Understood. Can you please close this pull request then? |
This option clashes with Drush 7.x and above and will not run.
https://github.com/drush-ops/drush/blob/master/commands/sql/sql.drush.inc#L560
Regards!
Jake