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

Require cell regex to be at beginning of line #1466

Closed
wants to merge 2 commits into from

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Nov 2, 2018

Currently, the regular expression used to delimit cells matches the comment symbol plus %% or other symbols anywhere on the line. That means that using export-notebook on the following splits the string:

# %%

import pandas as pd
import numpy as np

print('# %%')
print('this should be one cell')

Before:
image

After:
image

Presumably, run-cell would also fail on this.

@kylebarron
Copy link
Contributor Author

Do I need to use \\s to represent a space?

@n-riesco
Copy link
Collaborator

n-riesco commented Nov 2, 2018

On a regexp, no, but that's a string!

@kylebarron
Copy link
Contributor Author

So I do need to use \\s then?

@n-riesco
Copy link
Collaborator

n-riesco commented Nov 2, 2018

yes, we need to escape the backslash.

@kylebarron
Copy link
Contributor Author

Thanks. That does make sense.

@n-riesco
Copy link
Collaborator

n-riesco commented Nov 3, 2018

I can't look at this right now, but I don't think changing the regexp is the right approach.

I think we should update this part of the code to skip regexp matches for which isComment is false.

@kylebarron
Copy link
Contributor Author

That makes sense. I might try to find some time to look at that this weekend.

I'm also trying to figure out a good way to refactor some of this code that searches for code cell boundaries in order to carry some metadata about the cell, namely if it's a Markdown cell. Essentially all that's needed to fix #1296 is knowledge about which cells are Markdown and which are code.

@BenRussert
Copy link
Member

Good catch. I think the approach is fine, my only hesitation is @n-riesco's comment.

@BenRussert
Copy link
Member

Either way, it would be good to add a test to cover this.

@kylebarron
Copy link
Contributor Author

See #1520

@kylebarron kylebarron closed this Jan 28, 2019
kylebarron added a commit that referenced this pull request Feb 4, 2019
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.

None yet

3 participants