Implement `sys.getwindowsversion` by BasixKOR · Pull Request #2002 · RustPython/RustPython · GitHub
Skip to content

Implement sys.getwindowsversion - #2002

Merged
youknowone merged 7 commits into
RustPython:masterfrom
BasixKOR:sys-getwindowsversion
Jul 26, 2020
Merged

Implement sys.getwindowsversion #2002
youknowone merged 7 commits into
RustPython:masterfrom
BasixKOR:sys-getwindowsversion

Conversation

@BasixKOR

@BasixKOR BasixKOR commented Jul 16, 2020

Copy link
Copy Markdown
Contributor

This PR fixes #1996.

Note

In Windows 8.1 and Windows 10, the GetVersion and GetVersionEx functions have been deprecated. In Windows 10, the VerifyVersionInfo function has also been deprecated. While you can still call the deprecated functions, if your application does not specifically target Windows 8.1 or Windows 10, the functions will return the Windows 8 version (6.2). - Targeting your application for Windows

  • Since Windows 8.1 and later versions return 6.2 for non-manifested application, for now, this function will report the Windows version as 6.2 on modern systems. This can be fixed by adding a manifest with embed_resource crate.
  • platform_version returns the result from GetVersionExW, which is not accurate when running application in compatibility mode.

@coolreader18
coolreader18 requested a review from youknowone July 17, 2020 02:01
youknowone
youknowone reviewed Jul 17, 2020
View reviewed changes
Comment thread vm/src/sysmodule.rs Outdated
Comment thread vm/src/sysmodule.rs Outdated
youknowone
youknowone requested changes Jul 17, 2020
View reviewed changes

@youknowone youknowone left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this PR is working but we usually need a test to avoid regression. Will you please do one of below actions?

  1. Uncomment any corresponding part of unittest(Lib/test) if there are something
  2. Add a new test to tests/snippets/stdlib_os.py

Comment thread vm/src/sysmodule.rs Outdated

@youknowone youknowone Jul 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because this is not a documented or a CPython compitible attribute, naming to _getwindowsversion_type to mark it non-public and adding comment like // XXX: This is not a python spec but required by current RustPython implementation will be appreciated

@BasixKOR BasixKOR Jul 24, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just added some test cases and renamed the type. Could you check it again?

youknowone
youknowone requested changes Jul 24, 2020
View reviewed changes
Comment thread tests/snippets/stdlib_os.py Outdated
youknowone
youknowone reviewed Jul 24, 2020
View reviewed changes
Comment thread vm/src/sysmodule.rs Outdated
Ok(())
}

#[pystruct_sequence(name = "sys._getwindowsversion_type")]

@youknowone youknowone Jul 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actual name below is required to be aliased with _ prefix, but this one is not. This name can be sys.getwindowsversion without any problem.

To make things clear,

  • The name here is the type name.
  • The name below is the attrname of the sys module.

@BasixKOR BasixKOR Jul 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, thank you! I was bit confused about the differences between them.

youknowone
youknowone requested changes Jul 26, 2020
View reviewed changes
Comment thread vm/src/sysmodule.rs Outdated

@youknowone youknowone Jul 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you please rebase this PR to upstream? This macro have a bit different signature due to #2020.

@BasixKOR BasixKOR Jul 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would you mind if I rearrange commit history a little bit while rebasing? I'd like to clean up commits related to formatting.

@youknowone youknowone Jul 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cleaning up local history by rebasing is (almost) always recommended, welcome

@BasixKOR
BasixKOR force-pushed the sys-getwindowsversion branch from a928040 to 1505c9a Compare July 26, 2020 08:09
@BasixKOR
BasixKOR requested a review from youknowone July 26, 2020 08:21
youknowone
youknowone approved these changes Jul 26, 2020
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work!

@youknowone
youknowone merged commit 1fd8787 into RustPython:master Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

module 'sys' has no attribute 'getwindowsversion'

2 participants

@BasixKOR @youknowone