Update Copyright notice to remove year #3204
Conversation
This change is to fix Issue #3148 - Update copywrite notices.
| @@ -106,12 +106,13 @@ public int Start(string consoleFilePath, string[] args) | |||
| RunspaceConfigForSingleShell.Create(consoleFilePath, out warning); | |||
| } | |||
| int exitCode = 0; | |||
| string systemYear = DateTime.Now.Year.ToString(); | |||
There was a problem hiding this comment.
Do we really need this variable if it is used only in one place?
There was a problem hiding this comment.
I pulled it into a local variable because it's being used on line 113 and 115.
There was a problem hiding this comment.
Maybe I was not accurate. This variable is under the #if directive and actually used once in the result code.
|
Maybe we don't even need the year, that would be even simpler. |
|
I can change it to not have a year, but I was trying to fulfill what was asked in Issue #3148. There are no concerns of legal implications if the date isn't in there?
…________________________________
From: Jason Shirk <notifications@github.com>
Sent: Sunday, February 26, 2017 10:16 AM
To: PowerShell/PowerShell
Cc: Nicholas Willard; Author
Subject: Re: [PowerShell/PowerShell] Add dynamic year for managed entrance message. (#3204)
Maybe we don't even need the year, that would be even simpler.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#3204 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKg2vk1VPO9t9E_ME6YzRrOvu-t4aScYks5rgaVkgaJpZM4MMPrI>.
|
|
Currently all MSFT applications use a year in copyright (cmd.exe, notepad.exe, skype...) |
|
But they do not use a dynamic year - e.g. We can follow up, but it might just be a historical thing, or copying a pattern. There was a move to remove the copyright year from source files at one point. |
|
It seems that this (year) is tied to the main release time. I agree that is internal MSFT pattern. Perhaps it has historical legal roots. |
|
Aye, we need @joeyaiello and @SteveL-MSFT to weigh in on the legal question. |
|
I'll follow up on this. |
|
Thanks for all the comments and feedback. I out of habit brought my fork up to upstream/master, which, as it's supposed to, kicked off the CI builds. I didn't change any of the code I submitted. I'm just big on keeping my branch, fork in this case, up to date in order to limit merge conflicts. |
|
Just so I understand, is the date dynamically generated at build time or runtime? If I had to guess, I think runtime is probably problematic, but that's just a hunch. Either way, I'm sending off the mail to legal now |
|
@joeyaiello I've already started a conversation with legal |
|
@joeyaiello It is generated at runtime, which I can see how/why It'd be problematic. If legal decides that they want the date, I'll get it to generate at build time. I should've done that in the first place. |
|
Just found that powershell should go this direction too. |
|
We'll ask if that's an option to legal as well |
|
For legal too: What copyright we should use in the files? These files have been published under MIT. There are also new files. |
|
@SteveL-MSFT That's great news! I'll update it this evening and push the changes. Thanks everyone for all the collaboration. |
|
@SteveL-MSFT the changes have been made and this is ready for review. Thanks for being so helpful. |
| #else | ||
| var banner = ManagedEntranceStrings.ShellBanner; | ||
| var banner = string.Format(ManagedEntranceStrings.ShellBanner); |
There was a problem hiding this comment.
string.Format - It seems should be removed.
And why we use Core and non-Core banner?
|
@SteveL-MSFT Thanks for catching the unnecessary string.Formats. I forgot to remove them as well a couple using statements. The way I read the code when I first forked it, was that it shows a different banner based on the OS of the machine. Do you want me to remove that as well and just have it to be the same message regardless? |
|
I remember that @lzybkr open PR to remove Windows PowerShell from the repo so I believe we should remove the non-Core banner. |
|
@iSazonov I believe that PR is to just stop building Windows PowerShell. I would leave the non-Core banner for now. |
|
I agree. |
|
@kwiknick, |
|
Closed and Reopened to kick off the cla bot. |
This change is to fix Issue #3148 - Update copyright notices.
Changed the ManagedEntranceStrings resource file to take in a parameter. Then edited the ManagedEntrance.cs to grab the year using GetDate.Now and placing it in the banner variable for display when powershell is opened.