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

print newest deps #2792

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

Conversation

cdzwm
Copy link

@cdzwm cdzwm commented Apr 21, 2022

When conflict deps found, print the depency of newest version.

@technomancy
Copy link
Owner

Thanks for this patch.

I think overall it's good to surface this information, but the way it's presented in this implementation is not particularly clear. Simply saying Suggest: ... is confusing because it's presented alongside the suggestion to use :exclusions and it's not obvious how the two different suggestions are related. I think this needs some more thought put into it, but we could include it if the information is presented in a less confusing way.

There's also a couple technical issues with it; for one it removes :eval-in :leiningen which is pretty important, so that needs to be changed.

Also it appears to bring in a new dependency, which causes a lot of extra work for downstream packagers. As far as I can tell, this is only being used for a single function, which duplicates logic already available from Aether: https://github.com/technomancy/leiningen/blob/master/leiningen-core/src/leiningen/core/pedantic.clj#L68 Let's use the existing implementation instead of bringing in something redundant.

@cdzwm
Copy link
Author

cdzwm commented Apr 24, 2022

According to you suggestion, I make some modifications.

@cdzwm cdzwm closed this May 1, 2022
@technomancy
Copy link
Owner

Sorry, I haven't gotten a chance to look at this yet, but I don't think it needs to be closed. Was that intentional?

@cdzwm
Copy link
Author

cdzwm commented May 2, 2022

II'd like to think about this modification again. I think there are still some problems, but you can also take a look at it first and suggest some changes.

@cdzwm cdzwm reopened this May 2, 2022
@cdzwm
Copy link
Author

cdzwm commented May 17, 2022

I will make some fixes.

@cdzwm
Copy link
Author

cdzwm commented May 20, 2022

Fix is completed.

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