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

Fixes to changes in v20241208.001 related to pull request #62 #67

Closed
wants to merge 4 commits into from
Closed

Fixes to changes in v20241208.001 related to pull request #62 #67

wants to merge 4 commits into from

Conversation

sparrow2009
Copy link

This pull request fixes the misapplication of the code change proposed in #62.

In addition it also provides a fix to another issue (#62 (comment)) detected along the way when discussing #62: Option --perl was unconditionally turned on for commands installed and deps.

briandfoy
briandfoy requested changes Jan 28, 2025
View reviewed changes
Copy link
Owner

@briandfoy briandfoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why these are the correct changes, so you'll have to prove more to me and to demonstrate that each thing is incorrect.

lib/CPAN/Audit.pm Show resolved Hide resolved
Changes Show resolved Hide resolved
@briandfoy briandfoy added Status: changes requested adjust the pull request as noted in comments Type: bug an existing feature does not work Priority: high work on this first Status: needs details the problem description needs more information labels Jan 28, 2025
@briandfoy
Copy link
Owner

I think for each of your changes here, I need to see how you are using cpan-audit, what output you expected to get, what you got, and how this change fixes these.

And, since these are really three separate issues, separate PRs would be much better so I can deal with them one at a time.

sparrow2009 added 4 commits January 29, 2025 09:54
cmd installed: change version range operator to == for corelist'ed mo…
27da03e 
…dules

This only shows up as an issue if directories are passed on the command
line like in "cpan-audit installed local/". In that case @inc is not
inspected.
fix option --perl for command installed
623cb47 
On evaluation of $Module::CoreList::version{$]} some core modules
are mapped to distribution "perl" by module2dist().

Thus distribution "perl" is unconditionally included into the list
of inspected distributions even in the absence of option --perl.
support option --perl for command deps
1759ccd 
Fixing option --perl for command installed in the previous commit
also entailed the removal of distribution "perl" as a target
for inspection for command deps.

By adding support for option --perl also to the deps command the
old behaviour can be restored on request.
remove incorrect change log entry
8b0eb4a 
Remove change log entry "The default range operator is now `==` instead of
`>=`." for version 20241208.001.

The default range operator was not changed in that release.
@sparrow2009
Copy link
Author

sparrow2009 commented Jan 29, 2025 •
edited
Loading

Force pushed changes to commit meta data only.

@briandfoy briandfoy self-assigned this Jan 29, 2025
@briandfoy
Copy link
Owner

I'm going to reject this particular pull request and turn the various problem into separate issues so we can attack these things independently.

However, I'm leaning toward not making == the default and to leave the behavior as it is. I still don't have in my mind a good model of why the default should be == and I haven't seen a convincing argument from you. You can get want you want by specifying the == yourself on the command line, and I think that's preferable than changing how everyone else expects it to work based on its history.

@briandfoy briandfoy closed this Jan 29, 2025
@briandfoy briandfoy added Status: rejected this change is rejected and removed Priority: high work on this first Status: changes requested adjust the pull request as noted in comments labels Jan 29, 2025
@briandfoy briandfoy mentioned this pull request Jan 29, 2025
Closed
@briandfoy briandfoy removed the Status: needs details the problem description needs more information label Jan 29, 2025
@briandfoy briandfoy mentioned this pull request Jan 29, 2025
Open
@sparrow2009
Copy link
Author

I'm going to reject this particular pull request and turn the various problem into separate issues so we can attack these things independently.

Well, if you insist on your analysis that $dists is not prepopulated for the commands installed and deps (see above), the --perl option issue can not exist either. Because the former is the root of the latter.

... and I haven't seen a convincing argument from you. You can get want you want by specifying the == yourself on the command line, and I think that's preferable than changing how everyone else expects it to work based on its history.

This is confusing. Neither in #62 nor in #67 I intended, proposed or endorsed a change of the default version range operator to ==. That person was essentially you.

If I were in favour of that change I would have included a patch in this pull request in order to align the code behaviour with your false change log entry. But I did not and removed that change log entry instead.

All I proposed in #62 was to use the == version range operator for the installed command which does not even accept a version, thus also not a version range operator on the command line at all.

I think it was changed though? Can you say more about why you think it wasn't and where this would need to be fixed?

All I did was to be kind enough to outline to you the code changes necessary to make that default version range change to == happen. Nothing more and nothing less.

Repository owner locked and limited conversation to collaborators Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@briandfoy briandfoy briandfoy requested changes

Assignees

@briandfoy briandfoy

Labels
Status: rejected this change is rejected Type: bug an existing feature does not work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sparrow2009 @briandfoy