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.

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

Comment shebang line before passing to tokenizer #437

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

hdwalters
Copy link
Contributor

Fixes #435.

@Mte90
Copy link
Member

Mte90 commented Sep 2, 2024

Wondering if we can implement a test for this.
Also @Ph0enixKM what do you think of this change?

@Ph0enixKM
Copy link
Member

Ohhh yeah.. good point!

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

This fix seems to be good enough

@Ph0enixKM
Copy link
Member

@Mte90 I think that this was my fault. I forgot that this would alter the size of the output file.

if code.starts_with("#!") {
code.split('\n').skip(1).collect_vec().join("\n")
String::from("// ") + &code
Copy link
Member

Choose a reason for hiding this comment

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

Actually... can we replace shebang with an empty line? The comments can now persist in the generated bash file: #403

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but wouldn't it be better to show that there was a shebang in the input file, for debugging purposes?

Copy link
Member

@Ph0enixKM Ph0enixKM Sep 2, 2024

Choose a reason for hiding this comment

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

Okay let's leave it as a comment then

@Ph0enixKM
Copy link
Member

Just a small tweak so that we output a cleaner bash code

@hdwalters
Copy link
Contributor Author

hdwalters commented Sep 2, 2024

Wondering if we can implement a test for this.

I'm not sure that's going to work without changes to the test framework, as it does not currently appear to capture standard error:

hwalters@Wintermute ~/git/amber (comment-shebang-line) 
$ cat src/tests/validity/snippet_no_shebang.ab 
#!/usr/bin/env amber

// Output
//  ERROR  Function 'upper' does not exist
// at src/tests/validity/snippet_no_shebang.ab:12:6
// 
// 11| import { lower } from "std/text"
// 12| echo upper("shouty")
// 13| 

import { lower } from "std/text"
echo upper("shouty")
hwalters@Wintermute ~/git/amber (comment-shebang-line) 
$ amber src/tests/validity/snippet_no_shebang.ab 
 ERROR  Function 'upper' does not exist
at src/tests/validity/snippet_no_shebang.ab:12:6

11| import { lower } from "std/text"
12| echo upper("shouty")
13| 
hwalters@Wintermute ~/git/amber (comment-shebang-line) 
$ cargo test tests::validity::test_validity_src_tests_validity_snippet_no_shebang_ab
    Finished `test` profile [optimized + debuginfo] target(s) in 0.11s
     Running unittests src/main.rs (target/debug/deps/amber-67b01322e90afde0)

running 1 test
test tests::validity::test_validity_src_tests_validity_snippet_no_shebang_ab ... FAILED

failures:

---- tests::validity::test_validity_src_tests_validity_snippet_no_shebang_ab stdout ----
thread 'tests::validity::test_validity_src_tests_validity_snippet_no_shebang_ab' panicked at src/tests/mod.rs:22:21:
ERROR: Function 'upper' does not exist
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::validity::test_validity_src_tests_validity_snippet_no_shebang_ab

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 191 filtered out; finished in 0.03s

error: test failed, to rerun pass `--bin amber`

@Mte90
Copy link
Member

Mte90 commented Sep 2, 2024

I'm not sure that's going to work without changes to the test framework, as it does not currently appear to capture standard error:

Good point, we need something for that. What do you think @Ph0enixKM @b1ek

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Sep 2, 2024

Let's not bother testing formatting of errors just yet. Unless someone wants to implement them ofc. There are some more important things to be done now

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your help 🙏

@Ph0enixKM
Copy link
Member

@Mte90 that can be done with how we already validate the tests in stdlib and validity. We'd just check if the tests are failing and if so - with what error name.

I think that we should create error codes for different errors. This way we could just check which error code was triggered instead of checking the error message which could change anytime. I'll create issue for this

@Mte90 Mte90 merged commit addffab into amber-lang:master Sep 2, 2024
1 check passed
@hdwalters hdwalters deleted the comment-shebang-line branch September 2, 2024 09:40
Mte90 pushed a commit to Mte90/Amber that referenced this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Line number out by one for syntax errors in script with shebang
3 participants