Skip to content
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

Alternative Include-default tests #19

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kentfredric
Copy link

This branch incorporates the patches to Manifest.pm by @mohawk2 in pull #17.

However, it takes a different approach at testing, trying to put our best foot forwards in terms of not making the existing tests worse.

This sequence is of course, presently advisory, and commits can be squashed as needed.

Ideally I would make the file IO inside the test-case neater by having a tinyer version of Path::Tiny so you can more clearly see where the tests are happening and where the cruft is happening.

Additionally, this code would be substantially neater if dagolden/Test-TempDir-Tiny#7 was done.

My hope is this test will be 100% clear in exactly what it is testing, and why it is testing it.

There is by comparison nothing clear about: https://github.com/mohawk2/ExtUtils-Manifest/commit/4ab7d94a330848fb20d02bcbd5a5928d57ed3a9d#diff-58b9e890377f53ed47933c81a658843fR439

@mohawk2
Copy link
Member

mohawk2 commented Jul 5, 2015

I like what you've done here. How close are we to being ready to merge / release?

Dependency changes:

@@ -86,11 +86,11 @@
       "runtime" : {
          "requires" : {
             "Carp" : "0",
-            "Exporter" : "0",
+            "Exporter" : "5.57",
             "File::Basename" : "0",
             "File::Copy" : "0",
             "File::Find" : "0",
-            "File::Path" : "0",
+            "File::Path" : "2.01",
             "File::Spec" : "0.8",
             "perl" : "5.006",
             "strict" : "0",
@@ -102,11 +102,17 @@
             "CPAN::Meta" : "2.120900"
          },
          "requires" : {
+            "B" : "0",
             "Cwd" : "0",
             "Data::Dumper" : "0",
+            "Exporter" : "5.57",
             "ExtUtils::MakeMaker" : "0",
+            "File::Path" : "2.01",
             "File::Spec" : "0.8",
+            "File::Spec::Functions" : "0",
+            "File::Temp" : "0",
             "Test::More" : "0",
+            "lib" : "0",
             "perl" : "5.006"
          }
       }
@kentfredric
Copy link
Author

I've rebased this replacing the T:TD:T 0.004 with the pre-release of 0.005 ( w/ local merging ).

Effective dependency changes in the commit message.

the tests itself has been slightly restructured so the spew/slurp stages don't complicate understanding "What the test does"

@kentfredric
Copy link
Author

Its also possibly worth noticing that this patch by @mohawk2 does NOT change the behavior of "!include" ( Which it probably should ), but only changes the behavior of "!include_default".

I'll gladly attempt to fix that when this patch gets approved as changes relative to this one ( But It will necessitate adding a module with slurp/spew in it to share it nicely.

@kentfredric
Copy link
Author

Fun fact: Looks like I can stick File::Slurper in tlib/ without creating any more dependencies than Test::TempDir::Tiny already does, and it greatly simplifies code without needing me to re-invent and re-write how slurping works:

https://github.com/kentfredric/ExtUtils-Manifest/blob/fileslurper/t/maniskip.include-default.t

@kentfredric kentfredric force-pushed the includedefault branch 5 times, most recently from 573cedd to 606cfd4 Compare July 8, 2015 12:20
@kentfredric
Copy link
Author

NB: Githubs commit ordering is completely bollocks now.

29d7f44 Add Test::TempDir::Tiny 0.005-pre-release verbatim in t/tlib.
61d5833 Simplify unnecessary POD that wont get installed
f4e98f8 Add tlib/ByteSlurper.pm for reading/reading files as bytestrings
91b879e Add mailmap to map kentnl's CPAN email address
02d3e01 Doc, implement #!include_default as memory-only, not changing file.
d55d35a Add isolated tests for Mohawks include-default patch

This order is much more logical.

I've rebased/squashed things several times now:

Recent changes primarily include:

  • The factoring out of the spewing/slurping logic into its own module for re-use later
  • Nuking the unwanted POD from the stolen T:TD:T for a slimmer install ( and avoiding invalid POD ).

The "Nuke POD" change was done in an independent commit to make future updates to this module easier where necessary.

@mohawk2
Copy link
Member

mohawk2 commented Jul 9, 2015

I think #!include is probably intended to incorporate author-made contents for use at install-time. #!include_default is special because one can safely assume that at install-time, "default" will be available (and can change over time which is the motivation for the change), whereas separate author-made stuff won't be.

@haarg
Copy link
Member

haarg commented Jul 9, 2015

MANIFEST.SKIP doesn't matter at install time. It's only used to build the MANIFEST.

@mohawk2
Copy link
Member

mohawk2 commented Jul 10, 2015

Ah, that's right! #!include is also author-side only. Should I change that too?

@haarg
Copy link
Member

haarg commented Sep 8, 2015

@mohawk2 Yes, definitely.

@kentfredric
Copy link
Author

NB: I saw the IRC comment on this issue, but the box I run my irc stuff has now succumbed to hardware issues and no longer POSTs, so my IRC contact might be a little bit before its back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants