Skip to content
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

For # to start a comment, then it either need to be the first chara… #14562

Merged
merged 2 commits into from
Dec 13, 2024
Merged

For # to start a comment, then it either need to be the first chara… #14562

merged 2 commits into from
Dec 13, 2024

Conversation

RobbingDaHood
Copy link
Contributor

This PR should close

  1. Parsing validation fails with bareword containing # in block #10327
  2. Unbalanced delimiters on parens when stripping " from output of 2nd/last $"...(... | str trim -c '"')" before string ends #13667
  3. Cannot use command output as argument when the command includes # #13810
  4. potential parser bug when a bare string contains the "#" character #14129

Description

For # to start a comment, then it either need to be the first character of the token or prefixed with (space).

So now you can do this:

~/Projects/nushell> 1..10 | each {echo test#testing }                                                                                                                     12/05/2024 05:37:19 PM
╭───┬──────────────╮
│ 0 │ test#testing │
│ 1 │ test#testing │
│ 2 │ test#testing │
│ 3 │ test#testing │
│ 4 │ test#testing │
│ 5 │ test#testing │
│ 6 │ test#testing │
│ 7 │ test#testing │
│ 8 │ test#testing │
│ 9 │ test#testing │
╰───┴──────────────╯

User-Facing Changes

It is a breaking change if anyone expected comments to start in the middle of a string without any prefixing (space).

Tests + Formatting

Did all:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
  • cargo run -- -c "use toolkit.nu; toolkit test stdlib" to run the tests for the standard library

After Submitting

I cant see that I need to update anything in the documentation but please point me in the direction if there is anything.

@RobbingDaHood
Copy link
Contributor Author

These changes got rolled back because of an error: #14548

I fixed the error and I am trying again.

@RobbingDaHood
Copy link
Contributor Author

RobbingDaHood commented Dec 11, 2024 •
edited
Loading

@fdncred I did try testing #14548 (comment) but there is a lot of stuff I had to comment out: But it seemed to work.

Good thing that these syntax checks are early: There were no syntax errors left.

Could you check if these changes work for you now?

@RobbingDaHood
Copy link
Contributor Author

FYI: This test failed before the change: non_comment_hashtag_in_comment_does_not_stop_comment_in_block

@RobbingDaHood RobbingDaHood marked this pull request as ready for review December 11, 2024 17:45
@fdncred
Copy link
Collaborator

fdncred commented Dec 11, 2024

Thanks @RobbingDaHood. No problems with my mac nushell startup with this PR. Nice work!

I tested in release and debug on mac and debug on windows without issues. I don't use any of the nix tools though. Do you have time to try it out @sholderbach ?

@fdncred
Copy link
Collaborator

fdncred commented Dec 13, 2024 •
edited
Loading

Let's try again with time to dogfood a bit. Thanks!

@fdncred fdncred merged commit cbf5fa6 into nushell:main Dec 13, 2024
14 checks passed
@github-actions github-actions bot added this to the v0.101.0 milestone Dec 13, 2024
@sholderbach
Copy link
Member

Out of abundance of caution, could we add some tests for the things that broke for @fdncred with #14548 ?

@fdncred
Copy link
Collaborator

fdncred commented Dec 13, 2024

If I understand correctly, the blow test is what was causing it to fail for me.

nushell/tests/repl/test_parser.rs

Lines 182 to 192 in cbf5fa6

#[test]
fn non_comment_hashtag_in_comment_does_not_stop_comment_in_block() -> TestResult {
run_test(
r#"{
explore: {
# command_bar_text: { fg: '#C4C9C6' },
}
} | get explore | is-empty"#,
"true",
)
}

@fdncred
Copy link
Collaborator

fdncred commented Dec 17, 2024

@RobbingDaHood There's still a problem as described here #14605

def test [
] {
	# {
	# \""text"\"
	# `
}

# 123

@fdncred fdncred mentioned this pull request Dec 17, 2024
Merged
fdncred added a commit that referenced this pull request Dec 17, 2024
@fdncred
Revert "For # to start a comment, then it either need to be the fir…
d94b344 
…st chara…" (#14606)

Reverts #14562 due to #14605
@RobbingDaHood
Copy link
Contributor Author

RobbingDaHood commented Dec 18, 2024 •
edited
Loading

@RobbingDaHood There's still a problem as described here #14605

def test [
] {
	# {
	# \""text"\"
	# `
}

# 123

@fdncred who would have thought of hastags being prefixed with tabs 🙄

Found the solution, testing it now.

@RobbingDaHood
Copy link
Contributor Author

Fixed here: #14616

@hustcer hustcer removed this from the v0.101.0 milestone Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RobbingDaHood @fdncred @sholderbach @hustcer