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

Add "UTF8BOM" and "UTF8NoBOM" to the acceptable values for Encoding parameter. #59

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mkht
Copy link

@mkht mkht commented Jan 24, 2021

Pull Request (PR) description

Added UTF8BOM and UTF8NoBOM to the acceptable values for Encoding parameter to the KeyValuePairFile and ReplaceText.

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

@PlagueHO PlagueHO self-assigned this Oct 26, 2024
@PlagueHO
Copy link
Member

Hi @mkht - can you resolve conflicts and I'll review? Sorry about long delay.

@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Oct 26, 2024
@mkht mkht marked this pull request as draft October 26, 2024 15:55
@mkht mkht marked this pull request as ready for review October 26, 2024 16:03
@mkht
Copy link
Author

mkht commented Oct 26, 2024

I am surprised and pleased to see this project back in action.

We still use PowerShell DSC and this module almost every day. 😄

I would be happy to have you review it.

@johlju
Copy link
Member

johlju commented Oct 26, 2024

Can you change this line

dotnet tool install --global GitVersion.Tool

To:

dotnet tool install --global GitVersion.Tool --version 5.*

It will make the build pipeline work. There is a breaking change in v6 that is not yet supported by the pipeline.

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 89.70588% with 7 lines in your changes missing coverage. Please review.

Project coverage is 95%. Comparing base (0d1d026) to head (d30f23c).

Files with missing lines Patch % Lines
...s/FileContentDsc.Common/FileContentDsc.Common.psm1 92% 3 Missing ⚠️
...ces/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 88% 2 Missing ⚠️
.../DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 80% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@        Coverage Diff         @@
##           main   #59   +/-   ##
==================================
- Coverage    95%   95%   -1%     
==================================
  Files         4     4           
  Lines       264   304   +40     
==================================
+ Hits        253   290   +37     
- Misses       11    14    +3     
Files with missing lines Coverage Δ
...ces/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 95% <88%> (-2%) ⬇️
.../DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 97% <80%> (+2%) ⬆️
...s/FileContentDsc.Common/FileContentDsc.Common.psm1 88% <92%> (+5%) ⬆️

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Thank you for your patience @mkht - sadly I get almost no time on DSC anymore, but I try 😁

Reviewed 5 of 10 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: 6 of 11 files reviewed, 12 unresolved discussions (waiting on @mkht)


CHANGELOG.md line 8 at r3 (raw file):

## [Unreleased]

### Changed

Can you also add the change to the buildpipeline GitVersion.Tool to v5?


CHANGELOG.md line 26 at r3 (raw file):

- Automatically publish documentation to GitHub Wiki - Fixes [Issue #51](https://github.com/dsccommunity/FileContentDsc/issues/51).
- Renamed `master` branch to `main` - Fixes [Issue #53](https://github.com/dsccommunity/FileContentDsc/issues/53).
- Added `UTF8BOM` and `UTF8NoBOM` for Encoding parameter of the KeyValuePairFile and ReplaceText - Fixes [Issue #56](https://github.com/dsccommunity/FileContentDsc/issues/56).

nit:

Suggestion:

- Added `UTF8BOM` and `UTF8NoBOM` options to the Encoding parameter of the KeyValuePairFile and ReplaceText - Fixes [Issue #56](https://github.com/dsccommunity/FileContentDsc/issues/56).

source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 201 at r3 (raw file):

    Assert-ParametersValid @PSBoundParameters

    $fileEncoding = Get-FileEncoding $Path -ErrorAction SilentlyContinue

I notice this code block is almost identical to the code block in Test-TargetResource. (except this one uses ErrorAction SilientlyContinue). I'm wondering if:

  1. We shouldn't remove the ErrorAction SilentlyContinue (not sure why I did this at all - it worries me a bit).
  2. Refactor this into a Get-FileContent function to reduce code duplication but also make this easier to test.

What do you think?


source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 274 at r3 (raw file):

            {
                if ($PSBoundParameters.ContainsKey('Encoding') -and `
                    (($Encoding -eq $fileEncoding) -or `

Would it be possible to refactor this code logic into a separate function (like Test-FileCodingEqual or something). This would make it simpler to test?


source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 408 at r3 (raw file):

    }

    $fileEncoding = Get-FileEncoding $Path -ErrorAction SilentlyContinue

See comment above about possible reduction in code duplication by refactoring into a new function.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 45 at r3 (raw file):

    $fileEncoding = Get-FileEncoding $Path -ErrorAction SilentlyContinue
    if ($null -eq $fileEncoding)

See comment above about possible reduction in code duplication by refactoring into a new function.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 158 at r3 (raw file):

    $fileEncoding = Get-FileEncoding $Path -ErrorAction SilentlyContinue
    if ($null -eq $fileEncoding)

See comment above about possible reduction in code duplication by refactoring into a new function.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 290 at r3 (raw file):

    $fileEncoding = Get-FileEncoding $Path -ErrorAction SilentlyContinue
    if ($null -eq $fileEncoding)

See comment above about possible reduction in code duplication by refactoring into a new function.


source/Modules/FileContentDsc.Common/FileContentDsc.Common.psm1 line 180 at r3 (raw file):

    $ByteParam = if ($PSVersionTable.PSVersion.Major -ge 6)
    {
        @{AsByteStream = $true }

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    AsByteStream = $true
}

source/Modules/FileContentDsc.Common/FileContentDsc.Common.psm1 line 184 at r3 (raw file):

    else
    {
        @{Encoding = 'byte' }

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    Encoding = 'byte'
}

tests/TestHelpers/CommonTestHelper.psm1 line 115 at r3 (raw file):

    $ByteParam = if ($PSVersionTable.PSVersion.Major -ge 6)
    {
        @{AsByteStream = $true }

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    AsByteStream = $true
}

tests/TestHelpers/CommonTestHelper.psm1 line 119 at r3 (raw file):

    else
    {
        @{Encoding = 'byte' }

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    Encoding = 'byte'
}

Copy link
Author

@mkht mkht left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 11 files reviewed, 11 unresolved discussions (waiting on @PlagueHO)


CHANGELOG.md line 8 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you also add the change to the buildpipeline GitVersion.Tool to v5?

Done.


source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 201 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I notice this code block is almost identical to the code block in Test-TargetResource. (except this one uses ErrorAction SilientlyContinue). I'm wondering if:

  1. We shouldn't remove the ErrorAction SilentlyContinue (not sure why I did this at all - it worries me a bit).
  2. Refactor this into a Get-FileContent function to reduce code duplication but also make this easier to test.

What do you think?

Done.

Refactored to Get-FileContent, leaving SilentlyContinue in place.
It seems to me that SilentlyContinue was specified to avoid an error if the file does not exist. It might be better to be consistent since there are places in the code where the file is checked for existence in advance and other places where it is not.


source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 274 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Would it be possible to refactor this code logic into a separate function (like Test-FileCodingEqual or something). This would make it simpler to test?

Done.


source/DSCResources/DSC_KeyValuePairFile/DSC_KeyValuePairFile.psm1 line 408 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See comment above about possible reduction in code duplication by refactoring into a new function.

Done.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 45 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See comment above about possible reduction in code duplication by refactoring into a new function.

Done.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 158 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See comment above about possible reduction in code duplication by refactoring into a new function.

Done.


source/DSCResources/DSC_ReplaceText/DSC_ReplaceText.psm1 line 290 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

See comment above about possible reduction in code duplication by refactoring into a new function.

Done.


source/Modules/FileContentDsc.Common/FileContentDsc.Common.psm1 line 180 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    AsByteStream = $true
}

Done.


source/Modules/FileContentDsc.Common/FileContentDsc.Common.psm1 line 184 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    Encoding = 'byte'
}

Done.


tests/TestHelpers/CommonTestHelper.psm1 line 115 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    AsByteStream = $true
}

Done.


tests/TestHelpers/CommonTestHelper.psm1 line 119 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you adjust the formatting of this hashtable to fix the HQRM tests. It should be:

@{
    Encoding = 'byte'
}

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request needs a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to specify whether UTF8 files have a BOM or not
3 participants