-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Make Origins readonly #393
Make Origins readonly #393
Conversation
82c68c2
to
cf00991
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #393 +/- ##
==========================================
+ Coverage 76.80% 76.86% +0.05%
==========================================
Files 89 89
Lines 2531 2524 -7
Branches 427 426 -1
==========================================
- Hits 1944 1940 -4
+ Misses 466 463 -3
Partials 121 121
|
This solution to the #392 will cause a breaking change. That is fine since we will drop a v4 with other breaking changes. However, looking through this code, I'm wondering if we should take the opportunity clean up some of this mess.
That way we can just set it once in Origins setter. Any objections @aseigler? |
No objections, makes sense to me. |
Like this? Although FullyQualifiedOrigins might aswell be a { get; private set; } property now. Should I additionally document the breaking change somewhere? |
@Regenhardt It will be documented automatically in the Release notes. I will do a final review and merge as soon as I have a moment. Traveling this week. |
@Regenhardt This now has a bit of conflicts because we brought some older PRs in. Would you mind rebasing and sorting it out and we're pretty good to go? |
4fd115c
to
7faba0e
Compare
Rebased it |
Not sure what's going on, it looks like |
Re-ran all tests, seems good now. |
Closes #392