- Notifications
You must be signed in to change notification settings - Fork749
Drop Python 2 support#1158
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // We needs to replace all public constants to static readonly fields to allow | ||
| // binary substitution of different Python.Runtime.dll builds in a target application. | ||
| public static string pyversion => _pyversion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor I think a unit test depends on these public properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Which one? I was used in a few places, but only to check for Python 2 vs 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor you already removed them actually from TestPythonEngineProperties.cs
Seems like this might require an entry in the changelog since its a breaking change technically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I could do that, butpyversion etc. where never really consumables in the first place. I'd like to move the wholeRuntime tointernal as a single Changelog entry, no need to do this for every single component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor movingRuntime tointernal deserves its own tracking issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed:#1167
@filmor have you tried just changing the runtime and leaving the tests as-is for a first pass? I looked through the runtime and didn't see a mistake so I think I would start by ruling out problems with the test updating (which can easily be done in a followon branch) |
Not needed. The failures before were due to one place where I forgot to replace |
codecov-commenter commentedJun 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1158 +/- ##==========================================- Coverage 86.53% 86.25% -0.28%========================================== Files 1 1 Lines 297 291 -6 ==========================================- Hits 257 251 -6 Misses 40 40
Continue to review full report at Codecov.
|
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK by me after a couple minor fixes
| // We needs to replace all public constants to static readonly fields to allow | ||
| // binary substitution of different Python.Runtime.dll builds in a target application. | ||
| public static string pyversion => _pyversion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor movingRuntime tointernal deserves its own tracking issue.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Also, if you don't increase the coverage back to the target, or lower the target a bit, it is going to complain on every subsequent PR. |
The coverage is always against master, it will not repeatedly complain. |
Drop Python 2 support
What does this implement/fix? Explain your changes.
Drops support for and all references to Python 2.
Does this close any currently open issues?
Can't find it right now, but I think we had an issue discussing this.
Any other comments?
-/-
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG