-
Notifications
You must be signed in to change notification settings - Fork 717
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
Drop Python 2 support #1158
Drop Python 2 support #1158
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, but pyversion
etc. where never really consumables in the first place. I'd like to move the whole Runtime
to internal
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 moving Runtime
to internal
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 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.
|
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 moving Runtime
to internal
deserves its own tracking issue.
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.
AUTHORS
CHANGELOG