The Wayback Machine - https://web.archive.org/web/20201013013911/https://github.com/microsoft/PowerShellForGitHub/pull/245
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

Repair log file encoding on module load if required #245

Draft
wants to merge 8 commits into
base: master
from

Conversation

@themilanfan
Copy link
Contributor

@themilanfan themilanfan commented Jun 25, 2020

Description

Repair log file encoding from UTF16 to UTF8 when the module is loaded if required

Issues Fixed

Fixes #237

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • New/changed code continues to support the pipeline.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list
@HowardWolosky HowardWolosky added the bug label Jun 25, 2020
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Thanks for doing this.

A number of items for you to consider.

I also recommend that you come up with some manual test cases, list them out in your PR description, and validate that this change works across all of those tests cases (since we won't have UT's for it).

Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
PowerShellForGitHub.psd1 Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
PowerShellForGitHub.psd1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
@themilanfan
Copy link
Contributor Author

@themilanfan themilanfan commented Jun 26, 2020

@HowardWolosky thank you for the comments so far. I have yet to test, I just wanted to create a PR to show some progress.

I do have a question in regards to how you prefer PRs. Should I only create the PR after everything is tested and functioning or is it alright to create a draft PR just to show progress? I don't want to waste your time always if it's not complete but want to show progress, and also in case there could be valuable input to give along the way?

@HowardWolosky
Copy link
Member

@HowardWolosky HowardWolosky commented Jun 26, 2020

Thanks for asking! Draft PR's that are just showing progress are totally ok, however it's important to be clear about the state that it's in when you submit it.

So, if you're submitting it and haven't tested/verified it yet, please just explicitly say so. If you're not looking for a thorough code review and just want general confirmation that you're on the right track, say that. Just be clear with why you're submitting the PR, and that will tell me how thoroughly I should be looking at it.

Thanks for the help!

@themilanfan
Copy link
Contributor Author

@themilanfan themilanfan commented Jun 26, 2020

@HowardWolosky Thank you for the information!

themilanfan added 2 commits Jun 26, 2020
added try-catch for write line, moved onmoduleload to nested modules, updated comments
replace ::new(), add other try-finally, added Write-Log following repair
@themilanfan
Copy link
Contributor Author

@themilanfan themilanfan commented Jun 27, 2020

Pending testing

Helpers.ps1 Outdated Show resolved Hide resolved
Helpers.ps1 Outdated Show resolved Hide resolved
@themilanfan
Copy link
Contributor Author

@themilanfan themilanfan commented Jun 29, 2020

I will let you know once I've completed testing

@HowardWolosky
Copy link
Member

@HowardWolosky HowardWolosky commented Jun 29, 2020

I will let you know once I've completed testing

Sounds good. The code is looking pretty solid now, and I look forward to seeing your testing strategy.

@themilanfan
Copy link
Contributor Author

@themilanfan themilanfan commented Jul 6, 2020

Apologies for the delay, I was wrapping up my last 2 weeks of internship. I'll get on this hopefully shortly.

@HowardWolosky
Copy link
Member

@HowardWolosky HowardWolosky commented Aug 10, 2020

Hey @themilanfan -- Just wanted to check-in with you to see how things are going here.

Hope your internship went well!

Do you think you'll be coming back to this PR anytime soon?

@HowardWolosky
Copy link
Member

@HowardWolosky HowardWolosky commented Sep 9, 2020

Hey @themilanfan -- wanted to check-in to see if you think you'll be coming back to this PR?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.