Fix for Incorrect OS Version Displayed for Windows 11 in `marimo env` Command #1410

Haleshot · 2024-05-19T10:41:15Z

PR Title

Fix OS Version Display for Windows 11 in marimo env Command

Description

The current implementation of the get_system_info function in envinfo.py incorrectly retrieves the OS version using platform.release(), which does not differentiate between Windows 10 and Windows 11.. This PR addresses this issue by implementing a check for Windows 11 using sys.getwindowsversion().build and updating the OS Version field accordingly.
More information on this here.

Changes Made

Introduced a new function is_win11() to determine if the OS is Windows 11 based on the build number (sys.getwindowsversion().build >= 22000).
Modified get_system_info to use the is_win11() function to set the OS Version to "11" if running on Windows 11.

Example Output

Before PR:

{
  "marimo": "0.3.7",
  "OS": "Windows",
  "OS Version": "10",
  "Processor": "AMD64 Family 25 Model 80 Stepping 0, AuthenticAMD",
  "Python Version": "3.11.4",
  "Binaries": {
    "Browser": "--",
    "Node": "v18.17.0"
  },
  "Requirements": {
    "click": "8.1.6",
    "importlib-resources": "6.3.2",
    "jedi": "0.19.0",
    "markdown": "3.6",
    "pymdown-extensions": "10.7.1",
    "pygments": "2.15.1",
    "tomlkit": "0.12.4",
    "uvicorn": "0.29.0",
    "starlette": "0.37.2",
    "websocket": "missing",
    "typing-extensions": "4.10.0",
    "black": "24.3.0"
  }
}

After PR:

{
  "marimo": "0.3.7", 
  "OS": "Windows",
  "OS Version": "11",
  "Processor": "AMD64 Family 25 Model 80 Stepping 0, AuthenticAMD",
  "Python Version": "3.11.4",
  "Binaries": {
    "Browser": "--",
    "Node": "v18.17.0"
  },
  "Requirements": {
    "click": "8.1.6",
    "importlib-resources": "6.3.2",
    "jedi": "0.19.0",
    "markdown": "3.6",
    "pymdown-extensions": "10.7.1",
    "pygments": "2.15.1",
    "tomlkit": "0.12.4",
    "uvicorn": "0.29.0",
    "starlette": "0.37.2",
    "websocket": "missing",
    "typing-extensions": "4.10.0",
    "black": "24.3.0"
  }
}

Checklist

Implemented is_win11() function to correctly identify Windows 11.
Updated get_system_info to use is_win11() for determining the OS version.
Tested on Windows 11 to ensure correct functionality.

Fixes marimo-team#1409

vercel · 2024-05-19T10:41:20Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 20, 2024 6:08am
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 20, 2024 6:08am

github-actions · 2024-05-19T10:41:29Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

sourcery-ai

Hey @Haleshot - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 3 issues found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

marimo/_cli/envinfo.py

sourcery-ai · 2024-05-19T10:41:55Z

marimo/_cli/envinfo.py


 def get_system_info() -> dict[str, Union[str, dict[str, str]]]:
+    os_version = platform.release()


suggestion: Variable os_version might be redundant.

The os_version variable is only used once and could be inlined directly into the dictionary assignment for OS Version. This would reduce the number of lines and improve readability.

sourcery-ai · 2024-05-19T10:41:55Z

marimo/_cli/envinfo.py


 def get_system_info() -> dict[str, Union[str, dict[str, str]]]:
+    os_version = platform.release()
+    if platform.system() == "Windows" and is_win11():


suggestion (performance): Consider combining platform.system() checks.

The platform.system() check is performed twice in the function. Consider storing the result in a variable to avoid redundant calls and improve performance.

Suggested change

if platform.system() == "Windows" and is_win11():

system = platform.system()

if system == "Windows" and is_win11():

mscolnick · 2024-05-19T15:09:17Z

@Haleshot thanks for the contribution! i think some of the code-suggestions would be good to add, and there is a lint error (make py-check fixes this, and we have pre-commit too if you want to use that)

* adding reactive_html export * doctype to avoid quirks mode * respect config width * clarified help message * islands from_file static method * switching cli export to static method * adding ; between styles * removing exposing cli option --------- Co-authored-by: Myles Scolnick <myles@marimo.io>

Haleshot · 2024-05-20T02:13:58Z

recheck

mscolnick · 2024-05-20T02:16:48Z

@Haleshot main is currently failing so ill force merge, but you'll still need to sign the CLA aggrement (by commenting on this PR)

Haleshot · 2024-05-20T02:23:34Z

@Haleshot main is currently failing so ill force merge, but you'll still need to sign the CLA aggrement (by commenting on this PR)

Thanks. I seem to have combined another PR (didn't pull before committing) and even that commit got added here. Looking into removing that commit.

Apologize for the inconvenience caused.

This reverts commit ba2529a.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Haleshot · 2024-05-20T02:58:48Z

@mscolnick Hope it's alright now. Reverted the commit and now made the final changes. Do let me know if it's alright/anything to be done from my side.

mscolnick · 2024-05-20T03:09:31Z

@Haleshot you'll still need to sign the CLA - by pasting the text from above (it should be the first message in this thread)

Haleshot · 2024-05-20T03:11:28Z

I have read the CLA Document and I hereby sign the CLA

This reverts commit 1d953f3.

Fixes marimo-team#1413

Haleshot · 2024-05-20T06:16:57Z

Added PR template in the same. Fixes #1413
Do let me know if I should create a new PR for the above issue.

The merge-base changed after approval.

mscolnick · 2024-05-20T13:02:14Z

this PR template is great! Thank you. And thank you for the windows fix

github-actions · 2024-05-20T13:07:07Z

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.1-dev7

… Command (#1410)

gvarnavi · 2024-05-23T17:47:17Z

@mscolnick @akshayka This PR seems to have reverted the new MarimoIslandGenerator features.

mscolnick · 2024-05-23T20:28:39Z

@gvarnavi it looks like it did for a commit, but then that was unreverted. The PR diff still looks fine. Is the new feature not in the published version?

gvarnavi · 2024-05-23T20:36:54Z

It's not, no --that's how I caught it.
Here's the file history in main which shows that this was the last commit. Note it also affected marimo/_server/export/init.py.

akshayka · 2024-05-23T21:04:49Z

Weird .. I can revert it.

akshayka · 2024-05-23T21:14:11Z

It's not, no --that's how I caught it. Here's the file history in main which shows that this was the last commit. Note it also affected marimo/_server/export/init.py.

Let me know if #1471 looks good

Haleshot · 2024-05-24T02:28:59Z

Apologize for the above confusion and inconvenience caused due to this PR. Not sure how my commits got cluttered in the first place; learning to be more comfortable with using just git CLI than the VS integration.

Hope to be more careful when contributing next time.

gvarnavi · 2024-05-24T02:50:35Z

Oh no worries at all -- git merging can often feel like dark magic!

Incorrect OS Version Displayed for Windows 11 in marimo env Command

77e7705

Fixes marimo-team#1409

sourcery-ai bot reviewed May 19, 2024

View reviewed changes

Haleshot changed the title ~~Fix for Incorrect OS Version Displayed for Windows 11 in marimo env Command~~ Fix for Incorrect OS Version Displayed for Windows 11 in marimo env Command May 19, 2024

vercel bot deployed to Preview – marimo-storybook May 19, 2024 10:42 View deployment

vercel bot deployed to Preview – marimo-docs May 19, 2024 10:45 View deployment

gvarnavi and others added 3 commits May 19, 2024 13:16

Fix lint error and using pre-commit (basic issue resolving)

1a18b0f

Merge branch 'main' of https://github.com/Haleshot/marimo

a4e252d

vercel bot deployed to Preview – marimo-storybook May 20, 2024 02:09 View deployment

vercel bot deployed to Preview – marimo-docs May 20, 2024 02:09 View deployment

Haleshot added 3 commits May 20, 2024 08:19

Reviewed suggestions

4061661

Merge branch 'main' of https://github.com/Haleshot/marimo

c7a97cc

Revert "feat: adding reactive html export (marimo-team#1360)"

2f83522

This reverts commit ba2529a.

vercel bot deployed to Preview – marimo-storybook May 20, 2024 02:53 View deployment

vercel bot deployed to Preview – marimo-docs May 20, 2024 02:54 View deployment

Update marimo/_cli/envinfo.py

e3a05c6

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

vercel bot deployed to Preview – marimo-storybook May 20, 2024 02:58 View deployment

vercel bot deployed to Preview – marimo-docs May 20, 2024 03:00 View deployment

Haleshot added 2 commits May 20, 2024 10:51

Add Pull Request Template

1d953f3

Revert "Add Pull Request Template"

c6d8b8d

This reverts commit 1d953f3.

vercel bot deployed to Preview – marimo-docs May 20, 2024 05:24 View deployment

vercel bot deployed to Preview – marimo-storybook May 20, 2024 05:26 View deployment

Propose a New Pull Request Template for the Repository

36317c8

Fixes marimo-team#1413

vercel bot deployed to Preview – marimo-docs May 20, 2024 06:07 View deployment

vercel bot deployed to Preview – marimo-storybook May 20, 2024 06:08 View deployment

mscolnick previously approved these changes May 20, 2024

View reviewed changes

mscolnick merged commit 43a07c4 into marimo-team:main May 20, 2024
7 of 13 checks passed

mscolnick pushed a commit that referenced this pull request May 20, 2024

Fix for Incorrect OS Version Displayed for Windows 11 in marimo env…

6d3fa9a

… Command (#1410)

akshayka mentioned this pull request May 23, 2024

bring back island changes #1471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Incorrect OS Version Displayed for Windows 11 in `marimo env` Command #1410

Fix for Incorrect OS Version Displayed for Windows 11 in `marimo env` Command #1410

Haleshot commented May 19, 2024

vercel bot commented May 19, 2024 •

edited

github-actions bot commented May 19, 2024 •

edited

sourcery-ai bot left a comment

sourcery-ai bot May 19, 2024

sourcery-ai bot May 19, 2024

mscolnick commented May 19, 2024

Haleshot commented May 20, 2024

mscolnick commented May 20, 2024

Haleshot commented May 20, 2024

Haleshot commented May 20, 2024

mscolnick commented May 20, 2024

Haleshot commented May 20, 2024

Haleshot commented May 20, 2024 •

edited

mscolnick commented May 20, 2024

github-actions bot commented May 20, 2024

gvarnavi commented May 23, 2024

mscolnick commented May 23, 2024

gvarnavi commented May 23, 2024

akshayka commented May 23, 2024

akshayka commented May 23, 2024

Haleshot commented May 24, 2024

gvarnavi commented May 24, 2024


		def get_system_info() -> dict[str, Union[str, dict[str, str]]]:
		os_version = platform.release()

	if platform.system() == "Windows" and is_win11():
	system = platform.system()
	if system == "Windows" and is_win11():

Fix for Incorrect OS Version Displayed for Windows 11 in marimo env Command #1410

Fix for Incorrect OS Version Displayed for Windows 11 in marimo env Command #1410

Conversation

Haleshot commented May 19, 2024

PR Title

Description

Changes Made

Example Output

Checklist

vercel bot commented May 19, 2024 • edited

github-actions bot commented May 19, 2024 • edited

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

mscolnick commented May 19, 2024

Haleshot commented May 20, 2024

mscolnick commented May 20, 2024

Haleshot commented May 20, 2024

Haleshot commented May 20, 2024

mscolnick commented May 20, 2024

Haleshot commented May 20, 2024

Haleshot commented May 20, 2024 • edited

mscolnick commented May 20, 2024

github-actions bot commented May 20, 2024

gvarnavi commented May 23, 2024

mscolnick commented May 23, 2024

gvarnavi commented May 23, 2024

akshayka commented May 23, 2024

akshayka commented May 23, 2024

Haleshot commented May 24, 2024

gvarnavi commented May 24, 2024

Fix for Incorrect OS Version Displayed for Windows 11 in `marimo env` Command #1410

Fix for Incorrect OS Version Displayed for Windows 11 in `marimo env` Command #1410

vercel bot commented May 19, 2024 •

edited

github-actions bot commented May 19, 2024 •

edited

Haleshot commented May 20, 2024 •

edited