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.

Sign up for GitHub

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

Jump to bottom

add or retain an eval $VERSION when version has an underscore #11

Closed
wants to merge 3 commits into from
Closed

add or retain an eval $VERSION when version has an underscore #11

wants to merge 3 commits into from

Conversation

karenetheridge
Copy link
Contributor

Adds $VERSION = eval $VERSION; when using an underscore trial version; it turns out that Dist::Zilla is quite happy to deal with underscore versions everywhere; we just usually don't :)

(I'm doing a similar patch for PkgVersion; will link when it's up.)

This includes the commits for PR #10 to resolve conflicts. Please take that one first; it is much more important.

@karenetheridge karenetheridge force-pushed the topic/afterrelease-trial-comment-and-underscore-versions branch from f84fdac to c33b1e1 Compare June 27, 2015 21:00
@karenetheridge
Copy link
Contributor Author

PkgVersion modified similarly in rjbs/Dist-Zilla#466

@dagolden
Copy link
Collaborator

The logic is incorrect. The version shouldn't be eval'd if it's a tuple,
only a decimal.

Nor am I sold on doing anything to further encourage underscores, which
serve no purpose in a modern distribution.

That's why we have the #TRIAL comment and the -TRIAL suffix.

Cc: @rjbs
On Jun 27, 2015 3:19 PM, "Karen Etheridge" notifications@github.com wrote:

Adds $VERSION = eval $VERSION; when using an underscore trial version; it
turns out that Dist::Zilla is quite happy to deal with underscore versions
everywhere; we just usually don't :)

(I'm doing a similar patch for PkgVersion; will link when it's up.)

This includes the commit for PR #10
#10

to resolve conflicts. Please take that one first; it is much more important.

You can view, comment on, or merge this pull request online at:

#11
Commit Summary

  • protect new RELEASE_STATUS environment variable from interfering
    with tests
  • test for # TRIAL comment after [BumpVersionAfterRelease]
  • do not add # TRIAL to in-repo files after release
  • add or retain an eval $VERSION when version has an underscore

File Changes

Patch Links:

https://github.com/dagolden/Dist-Zilla-Plugin-BumpVersionAfterRelease/pull/11.patch

https://github.com/dagolden/Dist-Zilla-Plugin-BumpVersionAfterRelease/pull/11.diff

—
Reply to this email directly or view it on GitHub
#11
.

@karenetheridge
Copy link
Contributor Author

The logic is incorrect. The version shouldn't be eval'd if it's a tuple,
only a decimal.

Please clarify? '_' is not numeric, so whenever it is present, the version must be eval'd to remove it. (edit: I see what you mean; have fixed in a force-push.)

Nor am I sold on doing anything to further encourage underscores, which
serve no purpose in a modern distribution.

I used to believe this, but especially for dual-life modules, using an underscore to indicate a trial version is invaluable. It's the only way in which a version is obviously a trial version, e.g. when reading a list of installed modules in a smoke report.

I'm requesting this support be added here and in [PkgVersion] expressly to support the management of dual-life modules.

@karenetheridge
Copy link
Contributor Author

Ah I see what you mean about when to eval. Will fix.

@karenetheridge karenetheridge force-pushed the topic/afterrelease-trial-comment-and-underscore-versions branch from c33b1e1 to 3c91a6d Compare June 28, 2015 00:11
@dagolden
Copy link
Collaborator

Still wrong logic. Fails for v1.2_3. E.g.:

$ perl -wE 'my $v=version->new("v1.2_3"); say $v->is_alpha'
1

See how horrible underscores are? The number of people who know how to correctly deal with them can probably be counted on one hand and even they (me included!) get it wrong occasionally.

Even p5p can't seem to resist sticking underscores in things.

Underscores must be stopped. Version reports in smoke tests aren't a compelling reason to encourage further use.

@karenetheridge
Copy link
Contributor Author

Even p5p can't seem to resist sticking underscores in things.
Underscores must be stopped.

I've not seen this case be made anywhere yet, certainly not as vocally as the discussion around v-strings or two-dot versions.

@shadowcat-mst
Copy link

@dagolden Nor am I sold on doing anything to further encourage underscores, which
serve no purpose in a modern distribution.

You think they don't. I think they do. DBIx::Class relies on both, using underscores for alphas and TRIAL for RCs.

If you want to exclude underscore based versions you are intentionally ignoring a valid and long standing part of cpan for personal political reasons. I don't believe you've made that case.

@karenetheridge
Copy link
Contributor Author

There's legitimate advantages to underscore versions. Maybe there are bigger reasons to not use them. I'm willing to be persuaded, but that discussion needs to happen before action is taken.

@dagolden
Copy link
Collaborator

@shadowcat-mst If you want to exclude underscore based versions you are intentionally ignoring a valid and long standing part of cpan for personal political reasons

s/political/technical/

@shadowcat-mst
Copy link

@dagolden you keep saying that but I've offered to fix it technically repeatedly and every time I offer that you ignore it and then claim it's a technical question.

John Peacock refusing to listen to people is not a technical reason.

If you actually want to fix this, you've known for years I'd be happy to help.

If you don't, stop lying about the reasons for destroying a useful feature because you don't want to have the argument.

"I don't want to have the argument" is a valid reason. Claiming that's a technical rather than a political reason is not valid.

Pick a consistent position please.

@dagolden
Copy link
Collaborator

Technical rationale:

Please describe unambiguously for an 'average' Perl developer how to specify a dependency on a non-indexed module containing an underscore in its $VERSION.

Your answer should include:

  • How it should appear in Makefile.PL or Build.PL
  • How it should appear in a use line
  • How it should be given to a VERSION method call
  • How it should appear in a META file
  • Differences between decimal and tuple forms with underscore
  • Toolchain support considerations from 5.8.1 to the present
  • version.pm module considerations from 5.10.0 to the present

Extra credit is available for explaining how the above can be done without reading the dependency's source to see how they've defined $VERSION, including whether or not version.pm is loaded and whether or not an eval $VERSION is included.

Look – I think you'd acknowledge that I've dug into the details of $VERSION more than most, right? Even I don't think I can confidently answer the questions above. It is a technical mess.

However, not using underscores in versions makes it all easy. Want to specify a non-indexed version as a prerequisite? Just use it. In Makefile.PL. On a use line. It just works. Everywhere.

I think that's a powerful technical rationale: horrible complexity versus simplicity (except for the decimal vs tuple duality bit).

On consistency:

  • I've advocated against underscore tuples from the beginning
  • I've long pushed -TRIAL as a better way to tell PAUSE not to index
  • I pushed for 'release_status' in META with better granularity than binary indexed/not-indexed
  • I helped get the '# TRIAL' comment convention established so reading source can reveal stable/alpha status
  • I've argued that we should change version.pm semantics around underscores to alleviate some of the pain

So I think I've been consistent for a long time that attaching semantics to underscores is a bad idea.

Would I advocate that something like EUMM drop support for underscore (to the extent it has much support anyway)? No. I know CPAN has had underscores for ages. But I have no problem advocating against long-standing (bad) practice in a tool for authors.

I'm fine responding to a user's argument "please, I want to use underscores with this tool you wrote" with the response "no, you really don't".

I'm not going out filing bug tickets for every distro that uses underscore.

I'm saying "no, I don't want to include a feature to help authors make a bad decision".

@shadowcat-mst
Copy link

@dagolden I've offered to write this before now and you've said "but JPEACOCK won't accept it so I don't care".

So I call it political.

@ribasushi
Copy link

Can we make sure this ticket doesn't stale? The history of this debate is of no interest to most people on the sidelines.

The bullet list presented by @dagolden at the beginning of #11 (comment) is a reasonable list of things "the PTG at large must have answers to".

@shadowcat-mst would you be able to put together a technical answer to it? (assume JPEACOCK never existed, commits to both CPAN and p5p are non-problems, yadayada)

@dagolden
Copy link
Collaborator

After reflecting on this, I'm inclined to make users opt-in to decimal underscore support with a config option.

@karenetheridge
Copy link
Contributor Author

perhaps something like this if the config is not set and an underscore is not seen? karenetheridge/Dist-Zilla-Plugin-RewriteVersion-Transitional@676a4ff

(I haven't updated this PR to reflect the edits I made for PkgVersion, as I wanted to see where this discussion went.. but I think the PkgVersion changes are correct -- rjbs/Dist-Zilla#466 )

@karenetheridge karenetheridge force-pushed the topic/afterrelease-trial-comment-and-underscore-versions branch from 3c91a6d to ead6e11 Compare July 15, 2015 17:01
@shadowcat-mst
Copy link

@dagolden config option seems fine to me

@karenetheridge "warn loudly and mention the config option exists" would work for me

@karenetheridge
Copy link
Contributor Author

Note: I'm not in a hurry for this PR. I'd rather get it right and be slow than get it wrong and have to go fix downstream.

@dagolden
Copy link
Collaborator

Rebased, added config option and shipped to CPAN as -TRIAL

@dagolden dagolden closed this Jul 16, 2015
@karenetheridge
Copy link
Contributor Author

did you upload? I see nothing new on PAUSE.

@dagolden
Copy link
Collaborator

https://metacpan.org/release/DAGOLDEN/Dist-Zilla-Plugin-BumpVersionAfterRelease-0.010-TRIAL

@karenetheridge
Copy link
Contributor Author

ok, it's not on https://metacpan.org/recent nor did GumbyNET7 mention it on irc, so one of the data feeds must be down again.

@karenetheridge karenetheridge deleted the topic/afterrelease-trial-comment-and-underscore-versions branch July 16, 2015 16:49
@karenetheridge
Copy link
Contributor Author

I thought we'd decided on irc that inlining the munged version (a la tr/_//d) was preferable to an eval line? Also the eval does funky things with two-dot versions if I recall.

@dagolden
Copy link
Collaborator

The historical usage is "eval", so that's fine for this. Since it's only added for underscores and version tuples with underscores are prohibited, it will only ever exist for decimal underscore versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@karenetheridge @dagolden @shadowcat-mst @ribasushi