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

Make julia-latexsub work if point in the middle of latex symbol. #70

Closed
wants to merge 1 commit into from

Conversation

adam-higuera
Copy link

This pull request fixes two minor issues I had:

  1. emacs was erroring off because the invoke-command machinery was calling the zero-argument function julia-latexsub with a nil argument. Removing the i in the argument specifier string fixed this (might be a Spacemacs issue, not a julia-mode issue).
  2. It wasn't doing the substitution if point is in the middle of the latex command string.

The second change seems relatively innocuous, the first might break things that invoke julia-latexsub in ways that Spacemacs doesn't, i.e. with an argument (although it hasn't broken anything for me)

Copy link
Collaborator

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

LGTM, it would be great to merge this.

@tpapp tpapp self-requested a review October 3, 2019 15:53
@tpapp tpapp requested a review from rfourquet October 3, 2019 15:55
@rfourquet
Copy link
Collaborator

Unless I made a mistake, this doesn't work well for me: the substitution happens only if my point is within the word, not when it's at the end (in other words, the only thing which doesn't work is what was working before!). Can you confirm?

(interactive "*")
(let ((orig-pt (point))
(word-end (progn (forward-word)
(point))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is here, it goes forward one character too much.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will make it work

Suggested change
(point))))
(backward-char)
(point))))

@tpapp
Copy link
Collaborator

tpapp commented Nov 8, 2019

I think I can make it work with the suggestion above (currently testing it in practice).

But in general, I am a bit bothered by the fact that we don't have any unit tests for completion, so I will contribute some and in the meantime hold off on this PR.

@tpapp
Copy link
Collaborator

tpapp commented Nov 8, 2019

When #81 is merged, it should also be merged to this branch, and then some tests can be added (or commented out).

@non-Jedi
Copy link
Contributor

Superseded by #185

@non-Jedi non-Jedi closed this Mar 18, 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.

4 participants