Skip to content

Don't create new Solution snapshots if nothing changed#29583

Merged
jasonmalinowski merged 1 commit intodotnet:masterfrom
jasonmalinowski:skip-raise-of-workspace-changed-for-no-op
Aug 30, 2018
Merged

Don't create new Solution snapshots if nothing changed#29583
jasonmalinowski merged 1 commit intodotnet:masterfrom
jasonmalinowski:skip-raise-of-workspace-changed-for-no-op

Conversation

@jasonmalinowski
Copy link
Copy Markdown
Member

This just replicates the pattern used in a few other .With* methods in this type. A better fix would be to update ForkProject to do this automatically, but there's some complexity there as some calls require the fork to happen even if nothing did change. This is just a quick fix as the deeper fix is going to require some time and care.

This just replicates the pattern used in a few other .With* methods
in this type. A better fix would be to update ForkProject to do this
automatically, but there's some complexity there as some calls require
the fork to happen even if nothing did change. This is just a quick fix
as the deeper fix is going to require some time and care.
@jasonmalinowski jasonmalinowski requested a review from a team as a code owner August 29, 2018 18:11
@jasonmalinowski jasonmalinowski self-assigned this Aug 29, 2018
@sharwell
Copy link
Copy Markdown
Contributor

📝 We need to be certain the integration tests pass before merging this. The .editorconfig refresh functionality may be tied to this.

@jasonmalinowski
Copy link
Copy Markdown
Member Author

@sharwell Indeed, that special behavior is why I'm not pushing that into ForkProject for now. That logic is on a separate path, not these.

@jasonmalinowski
Copy link
Copy Markdown
Member Author

Specifically, see WithProjectOptionsChanged in that file just a bit farther down.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

there as some calls require the fork to happen even if nothing did change.

Can you clarifythat bit ? That sounds worrying :)

Comment thread src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs
@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Aug 29, 2018

Can you clarifythat bit ? That sounds worrying :)

.editorconfig isn't represented in the project system yet, so we detect changes through an external mechanism and trigger a solution change, even though the solution doesn't observe any known changes. This is how we know to re-run analyzers after .editorconfig changed.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

.editorconfig isn't represented in the project system yet, so we detect changes through an external mechanism and trigger a solution change, even though the solution doesn't observe any known changes. This is how we know to re-run analyzers after .editorconfig changed.

Thanks. That is indeed interesting and terrifying.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Is tehre any issue tracking having .editorconfig be tracked as part of the snapshot?

@sharwell
Copy link
Copy Markdown
Contributor

Is tehre any issue tracking having .editorconfig be tracked as part of the snapshot?

Yes, it's part of incorporating the editorconfig-in-compiler branch.

@jasonmalinowski
Copy link
Copy Markdown
Member Author

jasonmalinowski commented Aug 29, 2018

@dotnet-bot retest windows_release_unit64_prtest please. (Hit #29483.)

@jasonmalinowski
Copy link
Copy Markdown
Member Author

jasonmalinowski commented Aug 29, 2018

@dotnet-bot retest ubuntu_16_debug_prtest please. (@rainersigwald suggested a retry, since issue seems undiagnosable from the traces we have.)

@jasonmalinowski
Copy link
Copy Markdown
Member Author

jasonmalinowski commented Aug 29, 2018

@dotnet-bot retest ubuntu_16_debug_prtest please. (Broken by the razor compiler...uh...)

@jasonmalinowski jasonmalinowski merged commit e0e6181 into dotnet:master Aug 30, 2018
@jasonmalinowski jasonmalinowski deleted the skip-raise-of-workspace-changed-for-no-op branch August 30, 2018 18:09
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