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

[WIP] Give reasons for resolving group (#3008) #3302

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

Conversation

TeaDrivenDev
Copy link
Contributor

@TeaDrivenDev TeaDrivenDev commented Jul 14, 2018

This is a work in progress to discuss whether this is detailed enough, and a few questions I have about the code.

@TeaDrivenDev TeaDrivenDev changed the title [WIP] Give ) [WIP] Give reasons for resolving group (#3008) Jul 14, 2018
@@ -10,6 +10,8 @@ open Chessie.ErrorHandling
open Paket.Logging
open InstallProcess

type RTC = DependencyChangeDetection.ResolutionTriggeringChange
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only to bring in the type to avoid having to fully qualify

The alternatives I currently see are

  • opening the DependencyChangeDetection module; not sure if we'd want to do that here
  • specifying the type and module every time in the pattern match, which seems ugly
  • moving the original type somewhere else, like to Paket.Domain; not sure if that would be correct

@TeaDrivenDev
Copy link
Contributor Author

This addresses #3008 opened by @mavnn.

I have "surfaced" the information about specific changes that cause the resolver to be run for a group, and when that decision is made, every single individual reason for resolving is listed.

One question is what the desired amount of detail for the different cases is. Currently

  • all the different NuGet changes are listed individually; if there are multiple changes for a single package, all of them will be listed
  • if the framework restriction for a group changes, every single package will also be listed as having a framework restriction change; that probably doesn't make much sense
  • remote file changes will say "Remote file x changed in project y"
  • settings changes will only say "Group has settings changes"

Is there anything that should have more or less detail?

Ignore the individual commits and changes to files other than DependencyChangeDetection.fs and UpdateProcess.fs; I will rebase and clean those up later.

@@ -918,3 +918,5 @@ let main() =
else printError exn

main()

ignore ()
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@forki That's one of the things to.... ignore. ;-) I used that to set a final breakpoint for debugging and didn't want to sort out what to commit while this is still unfinished. As I said, I'll clean everything up when I finish the PR.

@mavnn
Copy link
Contributor

mavnn commented Jul 18, 2018

My personal feeling (@forki may have better ideas) is that if the resolver is triggered because of a change to paket.dependencies that's probably expected behaviour and shouldn't produce any output (except maybe with a verbose flag set?).

Apart from that, listing every package and why it's triggering the resolver sounds useful to me?

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