Remove duplication in MSFT_xWebsite.psm1 #384
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## dev #384 +/- ##
==========================================
+ Coverage 90.43% 90.61% +0.18%
==========================================
Files 17 17
Lines 2467 2409 -58
==========================================
- Hits 2231 2183 -48
+ Misses 236 226 -10
Continue to review full report at Codecov.
|
|
These PR is addressing problems of duplication mentioned in #241 |
There was a problem hiding this comment.
Thank you for fixing duplicate code! The tests passes so it seems this change works without issue.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @kmorcinek)
a discussion (no related file):
Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.
* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)
DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 304 at r1 (raw file):
} # Update Enabled Protocols if required
There are more here that seems duplicate? Would you like to resolve that to, or should we leave that for another PR?
johlju
added
the
waiting for code fix
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @kmorcinek)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue where applicable. You may suffix (it's optional) each entry with your name. Example of the format for the entry.
* Descriptive entry ([issue #123](https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/123). [Name/Alias (@github_account)](https://github.com/github_account)
Just added.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @johlju and @kmorcinek)
DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1, line 304 at r1 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
There are more here that seems duplicate? Would you like to resolve that to, or should we leave that for another PR?
I prefer to close this (and learn how to contribute - the full cycle), and then I will refactor the rest.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kmorcinek)
a discussion (no related file):
There was a new release of this resource module yesterday, because of that the change log is not up to date. You need to get the latest changes in the dev branch. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.
Works for me. Let's get this one to the finish line first. Almost there, just need the rebase as mention in previous comment :) |
kmorcinek
force-pushed
the
remove-duplication-in-xwebsitepsm1
branch
from
bf0c54f
to
f05d079
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @johlju)
a discussion (no related file):
Previously, kmorcinek (Krzysztof Morcinek) wrote…
Just added.
Done.
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
There was a new release of this resource module yesterday, because of that the change log is not up to date. You need to get the latest changes in the dev branch. Could you please rebase against branch dev using
git rebase(not by usinggit pullorgit merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.
Done. rebased and force pushed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)
a discussion (no related file):
Previously, kmorcinek (Krzysztof Morcinek) wrote…
Done. rebased and force pushed.
There was another PR merged, so now there is a merge conflict in the change log. Can you please rebase again to resolve the merge conflict?
kmorcinek
force-pushed
the
remove-duplication-in-xwebsitepsm1
branch
from
f05d079
to
7c86b20
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @johlju)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
There was another PR merged, so now there is a merge conflict in the change log. Can you please rebase again to resolve the merge conflict?
Done. rebased and force pushed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
johlju
added
needs review
|
@regedit32 Can you look this over and merge if you don't find any more review comments? |
|
LGTM |
We removed duplicated code in 'if' and 'else' branches and moved it behind if/else.
This change is