[stdlib] Add `os.path.expandvars` and respective tests #3735

thatstoasty · 2024-11-03T19:55:17Z

This PR adds the expandvars function to the os.path module.

The documentation for contributing references installing lit into our local Python environment, but with magic we have a Python environment already defined, so I added it as a dependency in the pixi.toml file. On a similar note, I added magic run to pre-commit hooks so magic is used for the Python and Mojo environments.

Any feedback on the changes or on the implementation would be welcome!

JoeLoser · 2024-11-04T18:24:53Z

magic.lock

@@ -119,6 +119,7 @@ environments:
      - conda: https://conda.anaconda.org/conda-forge/linux-64/libxcrypt-4.4.36-hd590300_1.conda
      - conda: https://conda.anaconda.org/conda-forge/linux-64/libxml2-2.13.4-h064dc61_2.conda
      - conda: https://conda.anaconda.org/conda-forge/linux-64/libzlib-1.3.1-hb9d3cd8_2.conda
+      - conda: https://conda.anaconda.org/conda-forge/noarch/lit-19.1.3-pyhd8ed1ab_0.conda


@Ahajha can you comment here how we're managing the magic lock files here internally wrt this? Given the migration to magic, I agree with @thatstoasty that we could just specify this as part of the dependencies in the pixi.toml.

I think this should be fine. 2 points:
1: This would allow a follow-up cleanup where we could remove the section of CI that installs lit through llvm.sh.
2: Internally, the lockfiles are wiped and then recreated each night (rather than updated in-place), so the only reason to update them here is to satisfy the --frozen flag, as pixi.toml is essentially ignored in CI here. (There might be a different flag that might be better for this use case, where it will check both?) After a night this should sync internally + regenerate, and then everything should be fine.

Verbal approval, from the CI side of things this should be a-ok.

@thatstoasty Thanks for improving the state of the world :)

Great! I handle a lot of the CI for my teams at work, so I know how fickle internal processes can be 🙂. Glad to see that the change might make its way downstream to make contributing a little easier!

JoeLoser

Great addition, @toiletsandpaper. LGTM, appreciate the easy-to-read test cases 👍

stdlib/src/os/path/path.mojo

stdlib/test/os/path/test_expandvars.mojo

JoeLoser · 2024-11-04T19:18:20Z

!sync

JoeLoser

LGTM! Just a minor remaining question/comment and then we can land this!

JoeLoser · 2024-11-05T04:32:02Z

stdlib/src/os/env.mojo

+    """Unsets an environment variable.
+
+    Constraints:
+        The function only works on macOS or Linux and returns False otherwise.


Question Any particular reason not to just use constrained instead (hard error in other words) instead of just gracefully returning False on Windows?

No concerns here, I was following the same pattern as getenv and setenv. I'll open up a separate PR to make them consistent with using constrained and c_int. For now I've updated only unsetenv.

stdlib/src/os/env.mojo

stdlib/src/os/path/path.mojo

martinvuyk

Hi @thatstoasty nice work. I left some ideas in case you want to apply them though some might take time and it's fine to leave them as is or do a followup (they're a bit of an overkill).

martinvuyk · 2024-11-05T17:12:54Z

stdlib/src/os/path/path.mojo

+    var b = int(byte)
+    return (
+        b == ord("_")
+        or ord("0") <= b
+        and b <= ord("9")
+        or ord("a") <= b
+        and b <= ord("z")
+        or ord("A") <= b
+        and b <= ord("Z")
+    )


Suggested change

var b = int(byte)

return (

b == ord("_")

or ord("0") <= b

and b <= ord("9")

or ord("a") <= b

and b <= ord("z")

or ord("A") <= b

and b <= ord("Z")

)

alias `_` = Byte(ord("_"))

alias `0` = Byte(ord("0"))

alias `9` = Byte(ord("9"))

alias `a` = Byte(ord("a"))

alias `z` = Byte(ord("z"))

alias `A` = Byte(ord("A"))

alias `Z` = Byte(ord("Z"))

var h_num = `0` <= byte <= `z` and not (`9` < byte < `A` or `Z` < byte < `a`)

return byte == `_` or h_num

Whenever I see backtick variables, it gets a bit difficult to differentiate between a variable name and a value. So I'm going to leave it as is for now. Perhaps it can be cleaned up as part of a future PR if backtick variables become the norm in the stdlib style guide

stdlib/src/os/path/path.mojo

martinvuyk · 2024-11-05T17:43:54Z

stdlib/src/os/path/path.mojo

+                if i == 1:
+                    return String("${}"), 2


Suggestion that might also beak some code or require some time

This is already handled by the logic bellow.

Suggested change

if i == 1:

return String("${}"), 2

The one after the while can also just return bytes[1:i]

return String("${"), 1

You can then change this:

# Invalid syntax (`${}` or `${`); write as is. if name.startswith("$") and length > 0: buf.write(name) # $ was not followed by a name, write the $. elif name == "": buf.write_bytes(bytes[j : j + 1])

to

# incomplete (`$` or `${}` or `${`); write as is. if length <= 2: buf.write_bytes(bytes[j:j + length + 1])

I was able to squash the cases into one:

# Invalid syntax (`${}` or `${`) or $ was not followed by a name; write as is. if name.startswith("{") or name == "": buf.write_bytes(bytes[j : j + length + 1])

if length <= 2: did not work for one letter environment variables like $a.

martinvuyk · 2024-11-05T18:05:49Z

stdlib/src/os/path/path.mojo

+    alias shell_variables = InlineArray[Int, 17](
+        ord("*"),
+        ord("#"),
+        ord("$"),
+        ord("@"),
+        ord("!"),
+        ord("?"),
+        ord("-"),
+        ord("0"),
+        ord("1"),
+        ord("2"),
+        ord("3"),
+        ord("4"),
+        ord("5"),
+        ord("6"),
+        ord("7"),
+        ord("8"),
+        ord("9"),
+    )
+    return int(byte) in shell_variables


Suggested change

alias shell_variables = InlineArray[Int, 17](

ord("*"),

ord("#"),

ord("$"),

ord("@"),

ord("!"),

ord("?"),

ord("-"),

ord("0"),

ord("1"),

ord("2"),

ord("3"),

ord("4"),

ord("5"),

ord("6"),

ord("7"),

ord("8"),

ord("9"),

)

return int(byte) in shell_variables

alias `0` = Byte(ord("0"))

alias `9` = Byte(ord("9"))

alias shell_variables = SIMD[DType.uint8, 8](

Byte(ord("!")),

Byte(ord("!")),

Byte(ord("#")),

Byte(ord("$")),

Byte(ord("*"),

Byte(ord("-")),

Byte(ord("?")),

Byte(ord("@")),

)

return `0` <= byte <= `9` or byte in shell_variables

I'm seeing some compilation issues using SIMD, so I'm going to leave it as it for now. Byte(ord(...)) was adding visual noise IMO.

…agic run` to pre-commit configuration. squash code a bit by using write add tests Cleanup with formatting and sign add missing newline Signed-off-by: Mikhail Tavarez <[email protected]> updated changelog

…agic run` to pre-commit configuration. squash code a bit by using write add tests Cleanup with formatting and sign add missing newline Signed-off-by: Mikhail Tavarez <[email protected]> updated changelog updated changelog Add unsetenv and use a context manager to unset test variables. Add renamed test_env.mojo Fix unsetenv result in EnvVar context manager. Signed-off-by: Mikhail Tavarez <[email protected]> use constrained for unsetenv use immutable stringslice

thatstoasty · 2024-11-06T16:49:09Z

@JoeLoser I think I'm good on additional changes to the code, unless you have some more suggestions!

JoeLoser · 2024-11-07T02:46:02Z

!sync

modularbot · 2024-11-07T03:38:36Z

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

…0283) [External] [stdlib] Add `os.path.expandvars` and respective tests This PR adds the `expandvars` function to the `os.path` module. The documentation for contributing references installing `lit` into our local Python environment, but with `magic` we have a Python environment already defined, so I added it as a dependency in the `pixi.toml` file. On a similar note, I added `magic run` to pre-commit hooks so `magic` is used for the Python and Mojo environments. Co-authored-by: Mikhail Tavarez <[email protected]> Closes #3735 MODULAR_ORIG_COMMIT_REV_ID: 2d8c7e8c2b2cadf773fcb36cdc9a33d27306c488

modularbot · 2024-11-07T06:20:12Z

Landed in dca55fd! Thank you for your contribution 🎉

…0283) [External] [stdlib] Add `os.path.expandvars` and respective tests This PR adds the `expandvars` function to the `os.path` module. The documentation for contributing references installing `lit` into our local Python environment, but with `magic` we have a Python environment already defined, so I added it as a dependency in the `pixi.toml` file. On a similar note, I added `magic run` to pre-commit hooks so `magic` is used for the Python and Mojo environments. Co-authored-by: Mikhail Tavarez <[email protected]> Closes #3735 MODULAR_ORIG_COMMIT_REV_ID: 2d8c7e8c2b2cadf773fcb36cdc9a33d27306c488

thatstoasty requested a review from a team as a code owner November 3, 2024 19:55

thatstoasty changed the title ~~Add os.path.expandvars and respective tests~~ [stdlib] Add os.path.expandvars and respective tests Nov 3, 2024

thatstoasty force-pushed the environ branch from 7c6fb7e to 0f28460 Compare November 3, 2024 20:06

thatstoasty marked this pull request as draft November 3, 2024 20:12

thatstoasty force-pushed the environ branch from 0f28460 to 9789a80 Compare November 4, 2024 16:28

thatstoasty marked this pull request as ready for review November 4, 2024 16:32

JoeLoser reviewed Nov 4, 2024

View reviewed changes

JoeLoser approved these changes Nov 4, 2024

View reviewed changes

stdlib/src/os/path/path.mojo Outdated Show resolved Hide resolved

stdlib/src/os/path/path.mojo Show resolved Hide resolved

stdlib/test/os/path/test_expandvars.mojo Outdated Show resolved Hide resolved

modular-automation bot assigned JoeLoser Nov 4, 2024

modularbot added the imported-internally Signals that a given pull request has been imported internally. label Nov 4, 2024

thatstoasty force-pushed the environ branch from f8332f6 to 884cb52 Compare November 4, 2024 21:09

thatstoasty mentioned this pull request Nov 4, 2024

[Feature Request] Ability to reserve String capacity directly #3738

Closed

1 task

JoeLoser approved these changes Nov 5, 2024

View reviewed changes

thatstoasty force-pushed the environ branch from 884cb52 to 20cc0ae Compare November 5, 2024 15:46

martinvuyk reviewed Nov 5, 2024

View reviewed changes

thatstoasty added 2 commits November 5, 2024 16:18

Add expandvars and respective tests. Added lit to pixi.toml, and `m…

6a5c2f7

…agic run` to pre-commit configuration. squash code a bit by using write add tests Cleanup with formatting and sign add missing newline Signed-off-by: Mikhail Tavarez <[email protected]> updated changelog

thatstoasty force-pushed the environ branch from d69f34e to 74621c4 Compare November 5, 2024 22:56

Merge branch 'nightly' into environ

0339b92

thatstoasty mentioned this pull request Nov 6, 2024

[stdlib] Addition of a new constructor and reserve method to the String struct #3751

Closed

modularbot added the merged-internally Indicates that this pull request has been merged internally label Nov 7, 2024

modularbot added the merged-externally Merged externally in public mojo repo label Nov 7, 2024

modularbot closed this Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] Add `os.path.expandvars` and respective tests #3735

[stdlib] Add `os.path.expandvars` and respective tests #3735

thatstoasty commented Nov 3, 2024

JoeLoser Nov 4, 2024

Ahajha Nov 4, 2024

thatstoasty Nov 4, 2024

JoeLoser left a comment

JoeLoser commented Nov 4, 2024

JoeLoser left a comment

JoeLoser Nov 5, 2024

thatstoasty Nov 5, 2024

martinvuyk left a comment

martinvuyk Nov 5, 2024

thatstoasty Nov 5, 2024 •

edited

Loading

martinvuyk Nov 5, 2024

thatstoasty Nov 5, 2024

martinvuyk Nov 5, 2024

thatstoasty Nov 5, 2024

thatstoasty commented Nov 6, 2024

JoeLoser commented Nov 7, 2024

modularbot commented Nov 7, 2024

modularbot commented Nov 7, 2024

[stdlib] Add os.path.expandvars and respective tests #3735

[stdlib] Add os.path.expandvars and respective tests #3735

Conversation

thatstoasty commented Nov 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLoser left a comment

Choose a reason for hiding this comment

JoeLoser commented Nov 4, 2024

JoeLoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvuyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatstoasty Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatstoasty commented Nov 6, 2024

JoeLoser commented Nov 7, 2024

modularbot commented Nov 7, 2024

modularbot commented Nov 7, 2024

[stdlib] Add `os.path.expandvars` and respective tests #3735

[stdlib] Add `os.path.expandvars` and respective tests #3735

thatstoasty Nov 5, 2024 •

edited

Loading