-
Notifications
You must be signed in to change notification settings - Fork 567
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 for an issue with empty math block #729
Conversation
So, it looks like your expected output for the modified test does not match the output that we should expect — which makes me think we need to reëvaluate that test, because it's not raising the error you mentioned in #726. |
@mpacer Not sure I got it. I added smoke test to make sure |
@mpacer I guess it's better now, but still little bit odd. With |
Why did you add the I think that's introducing your comment:
|
Ok, new thought, i don't know why it wasn't succeeding in the first place when it ran into a |
Ok, thinking this through a bit more… at the point that we're returning the value, the |
Ok, did a couple of quick tests to make sure that I was following this, we need the "" as a fallback in the case where either Since it was first triggered by the grammar match, we don't have to worry about returning an empty string in an inappropriate context. So we can safely always return a fallback empty string, and then if we pass the empty string, it should correctly be interpreted as So, the question remains: why did your the test you introduced in 4f23786 fail — it should have been passed through without issue. |
Ok, just double checked it; the issue with your initial commit was that you represented the multiline string as a single line. My guess is you were editing the file as a Anyway I created an example of how a notebook's cell should be formatted to pass the tests based on just opening and saving the notebook you modified in 4f23786. |
@mpacer Sorry, misunderstood your comment here #726 (comment) Thought it was about |
@mpacer Travis tests were failed with
Could you please restart them? I didn't figure out how to do it. |
I've restarted the failing jobs (@mpacer is probably asleep at the moment). |
Thanks a lot! |
Apologies @pacahon, @takluyver was correct, I was asleep. So… if our previous behaviour was to drop them, then we could continue to do that to be consistent with that. I'm not sure which is the more desired behaviour…. |
@mpacer By "them" you mean "$$$$"? Btw previous behaviour was |
I meant the previous behaviour as in removing Anyway I'm going to merge this and say it's fixed, and we'll iterate on this again if it comes up. |
Thanks. Since I made a mess in commit history, I little bit curious is it possible to squash commits when merging a PR? |
It is, though in this case @mpacer did a conventional merge instead. Here's what the interface looks like: |
@takluyver Thanks for the clarification. |
#726