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

Move the read_in and write_out subs into the module for code re-use #8

Merged
merged 3 commits into from Dec 12, 2017

Conversation

Projects
None yet
2 participants
@openstrike
Contributor

openstrike commented Sep 28, 2016

This change moves the two main input and output subroutines and their dependent private subs from the dpath script into the previously empty App::DPath module. The user-facing functionality of dpath is unchanged as is the test suite which still passes (for my installation at any rate). The benefit here is that any other script or module can now use the App::DPath module to read and write the structures to and from files with all the benefit provided by this dist without having to shell out or otherwise load the dpath script.

The PR isn't entirely finished yet - the POD could be improved and some tests written to access the subs directly instead of only via dpath. I'll be happy to polish it before merging if you were to approve the principle.

This work has been done as part of the CPAN PR Challenge. The dist was otherwise in great shape and it just seemed to me to be a pity that the module was essentially just a placeholder when some of the subs could be moved there for re-use.

@renormalist

This comment has been minimized.

Show comment
Hide comment
@renormalist

renormalist Oct 13, 2016

Owner

This patch totally makes sense. Thanks. Tell me when I should merge this PR (because you wrote "not entirely finished yet"). I'm also fine to merge this one right now and subsequent PRs later, too.

Owner

renormalist commented Oct 13, 2016

This patch totally makes sense. Thanks. Tell me when I should merge this PR (because you wrote "not entirely finished yet"). I'm also fine to merge this one right now and subsequent PRs later, too.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 15, 2016

Contributor

Glad you think so. I've started putting some tests together which have uncovered 1 bug already (in what I've done) so it would probably be best to hold off on merging until all the tests are done and pass. Hopefully be there within a week or so.

Contributor

openstrike commented Oct 15, 2016

Glad you think so. I've started putting some tests together which have uncovered 1 bug already (in what I've done) so it would probably be best to hold off on merging until all the tests are done and pass. Hopefully be there within a week or so.

Tests and POD for expanded module
Fix: move import of 'reftype' into module
Remove unused opt argument to _format_flat_inner_scalar
@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 24, 2016

Contributor

The tests are now done (see e728636) and the POD is a bit more extensive too. I think it is fine for merging now but if you think anything else needs attention please let me know.

Contributor

openstrike commented Oct 24, 2016

The tests are now done (see e728636) and the POD is a bit more extensive too. I think it is fine for merging now but if you think anything else needs attention please let me know.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 29, 2017

Contributor

Are you interested in merging this? If so and you let me know I'll resolve the merge conflicts and add the taparchive input type.

Contributor

openstrike commented Oct 29, 2017

Are you interested in merging this? If so and you let me know I'll resolve the merge conflicts and add the taparchive input type.

@renormalist

This comment has been minimized.

Show comment
Hide comment
@renormalist

renormalist Oct 29, 2017

Owner

Hoppla, I didn't see the update which github says was "1 year ago" - sorry. If you can resolve the conflicts, I will happily merge your work.

Owner

renormalist commented Oct 29, 2017

Hoppla, I didn't see the update which github says was "1 year ago" - sorry. If you can resolve the conflicts, I will happily merge your work.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Nov 1, 2017

Contributor

The conflicts should be resolved now. Please get back to me if you need anything further to merge this. Thanks.

Contributor

openstrike commented Nov 1, 2017

The conflicts should be resolved now. Please get back to me if you need anything further to merge this. Thanks.

@renormalist renormalist merged commit 2432694 into renormalist:master Dec 12, 2017

@renormalist

This comment has been minimized.

Show comment
Hide comment
@renormalist

renormalist Dec 12, 2017

Owner

Finally. Thanks!

Owner

renormalist commented Dec 12, 2017

Finally. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment