Skip to content

normalize special characters in module names to allow variable access #14353

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.

Sign up for GitHub

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

Jump to bottom
Merged
WindSoilder merged 1 commit into from
Dec 5, 2024
Merged

normalize special characters in module names to allow variable access #14353

WindSoilder merged 1 commit into from
Dec 5, 2024

Conversation

@sgvictorino
Copy link
Contributor

@sgvictorino sgvictorino commented Nov 15, 2024 •
edited
Loading

Fixes #14252

User-Facing Changes

  • Special characters in module names are replaced with underscores when
    importing constants, preventing "expected valid variable name":
> module foo-bar { export const baz = 1 }
> use foo-bar
> $foo_bar.baz
  • "expected valid variable name" errors now include a suggestion list:
> module foo-bar { export const baz = 1 }
> use foo-bar
> $foo-bar
Error: nu::parser::parse_mismatch_with_did_you_mean

  × Parse mismatch during operation.
   ╭─[entry #1:1:1]
 1 │ $foo-bar;
   · ────┬───
   ·     ╰── expected valid variable name. Did you mean '$foo_bar'?
   ╰────

@sholderbach sholderbach added notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes deprecated:pr-language (deprecated: this label is too vague) This PR makes some language changes labels Nov 15, 2024
@fdncred
Copy link
Contributor

fdncred commented Nov 15, 2024

@sgvictorino is on fire 🔥!!! We appreciate all your PRs. Thanks.

@WindSoilder
Copy link
Contributor

WindSoilder commented Nov 17, 2024

Thanks! But there is one thing I'm not sure:

module foo-bar { export const baz = 1 }
use foo-bar
$foo-bar.baz

Personally I think it's better to export use baz variable like $foo_bar.baz, to user, after using foo-bar and using it with $ sign, it's using more like a variable rather than a module name.

@sholderbach
Copy link
Member

sholderbach commented Nov 17, 2024

How will this play out with use <filename>.nu modules?

@sgvictorino
Copy link
Contributor Author

sgvictorino commented Nov 17, 2024 •
edited
Loading

How will this play out with use <filename>.nu modules?

The filename stem must be a valid module name. Out of the invalid identifier characters, someone might try to name files like foo.bar.nu, but fully qualified access would already be ambiguous with a cell path on $foo.

I'm also fine with normalizing certain symbols to a valid variable name like @WindSoilder prefers. The "did you mean" fuzzy search helps with that UX.

@132ikl
Copy link
Member

132ikl commented Nov 19, 2024

I think normalizing would be the best option, since then it can align with typical variable name rules and can possibly catch more cases than just hyphens and the dot issue you bring up.

@WindSoilder
Copy link
Contributor

WindSoilder commented Nov 22, 2024

Yeah, @sgvictorino can you update the code to use $foo_bar.baz?

@sgvictorino sgvictorino force-pushed the stricter-module-name-parsing branch 3 times, most recently from 99f4337 to c446b74 Compare November 22, 2024 22:11
@sgvictorino sgvictorino changed the title make module name parsing more strict normalize special characters in module names to allow variable access Nov 22, 2024
WindSoilder
WindSoilder requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Sorry for a delay. Finally I take time to review it.

And I think the solution can be easier.

@sgvictorino sgvictorino force-pushed the stricter-module-name-parsing branch 2 times, most recently from 44f74bc to 5ddc69d Compare November 30, 2024 19:27
WindSoilder
WindSoilder requested changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@WindSoilder WindSoilder left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Just one thing left.

@sgvictorino
normalize special characters in module names to allow variable access
7aedc8b 
# User-Facing Changes

- Special characters in module names are replaced with underscores when
  importing constants, preventing "expected valid variable name":

```nushell
> module foo-bar { export const baz = 1 }
> use foo-bar
> $foo_bar.baz
```

- "expected valid variable name" errors now include a suggestion list:

```nushell
> module foo-bar { export const baz = 1 }
> use foo-bar
> $foo-bar
Error: nu::parser::parse_mismatch_with_did_you_mean

  × Parse mismatch during operation.
   ╭─[entry #1:1:1]
 1 │ $foo-bar;
   · ────┬───
   ·     ╰── expected valid variable name. Did you mean '$foo_bar'?
   ╰────
```
@sgvictorino sgvictorino force-pushed the stricter-module-name-parsing branch from 5ddc69d to 7aedc8b Compare December 2, 2024 17:48
WindSoilder
WindSoilder approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@WindSoilder WindSoilder merged commit 234484b into nushell:main Dec 5, 2024
14 checks passed
@github-actions github-actions bot added this to the v0.101.0 milestone Dec 5, 2024
@sgvictorino sgvictorino deleted the stricter-module-name-parsing branch December 13, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WindSoilder WindSoilder WindSoilder approved these changes

Assignees

No one assigned

Labels

deprecated:pr-language (deprecated: this label is too vague) This PR makes some language changes notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

None yet

Milestone

v0.101.0

Development

Successfully merging this pull request may close these issues.

Exported constants in modules with a dash in the name aren't accessible

5 participants

@sgvictorino @fdncred @WindSoilder @sholderbach @132ikl