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

Load project to hold ruleset compilation options #1440

Merged
merged 58 commits into from
Jun 17, 2019

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Mar 31, 2019

Resolves one review issue from original PR #1076 / first two bullets from #1301 and #1458.

This version loads rulesets as part of project instead of joining them to project on every analysis routine.

End user gets:

  • Dynamically loaded / modifiable rulesets instead without full restart on omnisharp after every change.
  • Reanalyze updated projects.
  • CS0000 diagnostics can be controlled by rulesets even when analyzers are not enabled.
  • (Very) slightly faster analysis since project isn't updated every time.
  • When project is restored -> it is re-analyzed with correct depencies.

Pending

  • Tests (fix existing + add new that tests for project loader).
  • Tweaks

@savpek savpek marked this pull request as ready for review April 5, 2019 11:45
@savpek
Copy link
Contributor Author

savpek commented Apr 5, 2019

@rchande @filipw @mholo65 @david-driscoll ready for review.

@savpek savpek changed the title Load rulesets to projects Load project to hold ruleset compilation options Apr 5, 2019
@filipw
Copy link
Member

filipw commented Apr 28, 2019

Marking as blocked until #1474 (so that this one goes first).

@savpek
Copy link
Contributor Author

savpek commented May 8, 2019

Ready for re-review @rchande @filipw

Current behavior:
updatable-rulesets

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@filipw
Copy link
Member

filipw commented May 8, 2019

@savpek if you don't mind, it should be squashed on merge

@savpek
Copy link
Contributor Author

savpek commented May 9, 2019

@savpek if you don't mind, it should be squashed on merge

Squashing is fine 🙂

@filipw
Copy link
Member

filipw commented May 27, 2019

@rchande @david-driscoll @mholo65 could you guys please have a look? imho should be good to go

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

Looks good to me. My comments are just nitpicking.

src/OmniSharp.MSBuild/ProjectManager.cs Outdated Show resolved Hide resolved
@filipw filipw requested a review from rchande June 3, 2019 12:31
@david-driscoll david-driscoll dismissed rchande’s stale review June 16, 2019 23:46

Looks like the changes have been addressed

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Looks good to me, looks like @rchande changes have been addressed as well. Going to update the branch to ensure there are no conflicts.

@filipw filipw merged commit af17b42 into OmniSharp:master Jun 17, 2019
@filipw
Copy link
Member

filipw commented Jun 17, 2019

thanks!

@filipw
Copy link
Member

filipw commented Jun 17, 2019

Squashing is fine 🙂

forgot to do it 😄

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.

5 participants