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

Fix shebang handling for languages with non-# comments #434

Merged
merged 1 commit into from
Feb 13, 2020
Merged

Fix shebang handling for languages with non-# comments #434

merged 1 commit into from
Feb 13, 2020

Conversation

jonasbb
Copy link
Contributor

@jonasbb jonasbb commented Feb 13, 2020

The shebang always starts with the sequence #! independent of the comment symbol of the programming language. Add a test with Rust, which uses // as the comment symbol.

The problem with the existing code is that it uses the comment symbol in the shebang line. The shebang can only start with #! though. This leads to two problems.
When parsing the shebang, when converting from text to notebook format, it uses a fixed offset of two characters to take the executable path (line[2:]), which fails if the comment is not only one character, like the two characters of //.
Similarly, when converting from notebook to text, it uses the comment symbol again and writes an invalid shebang.

The new test is mostly copied from the Python version with slight adaptions.

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #434 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
+ Coverage   98.94%   98.94%   +<.01%     
==========================================
  Files          75       75              
  Lines        7646     7651       +5     
==========================================
+ Hits         7565     7570       +5     
  Misses         81       81
Impacted Files Coverage Δ
tests/test_read_simple_rust.py 100% <100%> (ø) ⬆️
jupytext/header.py 99.23% <100%> (ø) ⬆️
tests/test_pep8.py 98.38% <0%> (ø) ⬆️
tests/test_cli.py 100% <0%> (ø) ⬆️
tests/test_black.py 100% <0%> (ø) ⬆️
tests/test_active_cells.py 100% <0%> (ø) ⬆️
tests/test_knitr_spin.py 100% <0%> (ø) ⬆️
tests/test_compare.py 100% <0%> (ø) ⬆️
tests/test_ipynb_to_rmd.py 100% <0%> (ø) ⬆️
tests/conftest.py 100% <0%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcf1a7b...8773413. Read the comment docs.

@mwouts
Copy link
Owner

mwouts commented Feb 13, 2020

Oh that's interesting! Thank you @jonasbb for pointing this out.

@@ -144,7 +144,7 @@ def header_to_metadata_and_cell(lines, header_prefix, ext=None):
encoding_re = re.compile(r'^[ \t\f]*{}.*?coding[:=][ \t]*([-_.a-zA-Z0-9]+)'.format(comment))
Copy link
Owner

Choose a reason for hiding this comment

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

Regarding the encoding, do you know it it should use the language line comment, or #?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only know the encoding line from Python 2 programs, so I am not sure if other languages use this feature as well.


# remove version information
def remove_version_info(text):
return '\n'.join([line for line in text.splitlines() if 'version' not in line])
Copy link
Owner

Choose a reason for hiding this comment

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

For this, typically I use the fixture no_jupytext_version_number (you just need to add that argument to the test function, and remove the version information in the text)... If that's not clear I can also do it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know this feature exists. I push a new version to use it.

FYI: I copied this test, which is where I found this remove_version_info function.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks! Oh interesting, later on I will update that other test.

The shebang always starts with the sequence "#!" independend of the
comment symbol of the programming language.
Add a test with Rust, which uses "//" as the comment symbol.
@mwouts mwouts merged commit d6abab5 into mwouts:master Feb 13, 2020
@mwouts mwouts added this to the 1.3.4 milestone Feb 13, 2020
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.

2 participants