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

Insert references before local variables #262

Merged
merged 1 commit into from
Oct 1, 2017
Merged

Conversation

phst
Copy link
Contributor

@phst phst commented Sep 23, 2017

Fixes #216

@jrblevin
Copy link
Owner

Thanks for this (and your many other PRs)! Generally this seems fine, but I have two thoughts:

  1. Shouldn't this only search for local variables in the range (max (- (point-max) 3000) (point-min)) to (point-max)? Ignoring the (max ... (point-min)) part for simplicity, as I read the patch it looks from (point), the proposed reference definition location to (point) minus 3000. The difference is that I think it should start from (point-max) and probably only execute if the proposed point is somewhere in that range.

  2. Since this is based on a simple string search, it would be "fooled" by a local variables block inside a code block, such as this one. To protect against this, I think you could use markdown-search-until-condition with markdown-conditional-search-function let-bound to search-backward and with the condition being that the match is not inside a code block—something like(lambda () (not (markdown-code-block-at-point-p))).

If the first one is fixed, that would at least minimize the chances of being affected by the second (i.e., only a code block near the end of the file would fool it).

@phst
Copy link
Contributor Author

phst commented Oct 1, 2017

The difference is that I think it should start from (point-max) and probably only execute if the proposed point is somewhere in that range.

That should already be the case by virtue of checking for eobp in line 4381.

Since this is based on a simple string search, it would be "fooled" by a local variables block inside a code block

True, but that's already the case for other uses of local variables. For example, try visiting a file containing

``` html
<!-- Local Variables: -->
<!-- tab-width: 17 -->
<!-- End: -->
```

After visiting this file, tab-width will indeed be 17. So while not perfect, it's at least consistent with other uses of file-local variables.

@jrblevin
Copy link
Owner

jrblevin commented Oct 1, 2017

The difference is that I think it should start from (point-max) and probably only execute if the proposed point is somewhere in that range.

That should already be the case by virtue of checking for eobp in line 4381.

You’re right, thanks. I missed that somehow.

@jrblevin jrblevin merged commit bc79248 into jrblevin:master Oct 1, 2017
@phst phst deleted the bug216 branch October 1, 2017 14:32
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