Test, doc, implement #!include_default as memory-only, not changing disk file #17
Conversation
|
@mohawk2 Next time, push your branch directly in the main repo and create your PR from this project: as you have a commit bit you don't need a GitHub fork. |
|
I think we can ignore the local $_ issue for now, since it's already a problem in numerous other places in this module. If we want to clean up those parts and possibly modernize other parts of the code, that can be handled separately. +1 on this PR |
|
A thought: It may be worth investigating how hard it is to write the test parts for this in their own file.
Because I'm currently of the mind that if we can never touch Manifest.t ever again, it will be a net benefit: Because there's far too much risk in losing some important test coverage in my translation, due to not understanding what the code did on some ancient And subsequently, it would be better to avoid building on this mess for future enhancements as much as possible. My branch, in reflection, is far too much of an ambitious experiment to expect it to be in EUM any time soon, and assuming we can mitigate this problem in a more legacy friendly way by not touching the file, becomes something that really shouldn't block progress. |
|
In my view, adding 7 lines to the end of an existing test file, which cannot interfere with the operation of the existing tests, is not something that demands a root-and-branch replacement of those tests. This change should be judged on its own merits. On my understanding of the discussion over this feature (years ago), it was considered a worthwhile change to make. I have yet to see substantive requests for changes to this change-set. Surely this suggests it is merge-worthy? |
Maybe you're not comprehending how much of a shitmess the tests are then. |
|
And, EVEN if it was inconsequential to add them to an existing test, it is STILL desirable to have a dedicated test for this feature. There are so many advantages to adding tests files instead of extending existing ones ( especially when it comes to reverting a change after other changes are made ) |
The change is also incomplete at present. And presently, for whatever reason, maniskip() ->('Makefile') doesn't return true for me, and my tests are failing for some reason. |
|
Any progress about this PR? |
I've just rebased it (the tab-removing caused merge conflicts). @haarg and @karenetheridge - is this fit to merge? |
|
This may be a bad idea for reproducibility, it will make the resulting tarball dependent on the version of ExtUtils::Manifest, something it is not today. If we really want this I would suggest giving it a different name than the existing semantics, so that it doesn't break anything. But probably we should come up with a design that's both updatable and reproducible. Alternatively, having a better way to do reproducible build could solve that too. |
On second thought this is probably my preferred solution anyway, so I'm fine with merging this but will post a follow-up to solve my problem soon. |
|
I am very supportive of reproducible builds. My understanding of @Leont 's thought is he will follow up with improvements to enable that - if so, @karenetheridge are you comfortable merging this? |
|
Wow, I'm actually reviewing this! Not in this PR -- but since the manifest is now being expanded in-memory, it's no longer as easy to see what it actually expands to, so it would be nice to have some sort of mechanism (driven by environment variable, perhaps?) to write out the file that would have been created before, to serve as a sanity check. |
|
🥳 |
|
and sorry that this took so long... PTS yay for forcing me to deal with things. |
No description provided.