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

ctags: fix Ruby parsing for block assigns #1820

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tambeta
Copy link

@tambeta tambeta commented Apr 6, 2018

Assigning blocks' return values to variables is a common idiom in Ruby.
Fix the ctags parser to take this into account. Closes #1744.

Assigning blocks' return values to variables is a common idiom in Ruby.
Fix the ctags parser to take this into account. Closes geany#1744.
@elextr
Copy link
Member

elextr commented Apr 6, 2018

@tambeta thanks, does this apply to upstream universal ctags if so probably best to submit it there and then make a PR to pull the modified parser into Geany (unless Geany one has other fixes uctags doesn't).

The block assign fix (geany#1744) was submitted and accepted to
universal-ctags. Pull in the relevant changes.
@tambeta
Copy link
Author

tambeta commented Apr 27, 2018

@elextr, I'm not entirely sure what's the protocol for syncing parser changes from upstream, but I submitted this fix to universal-ctags and it was accepted (universal-ctags/ctags#1734).

I've made another commit on this branch pulling in the relevant changes. The improvements can be reviewed using this Ruby test file - using various assignments with blocks no longer breaks the symbols tree.

@kugel-
Copy link
Member

kugel- commented May 6, 2018

LGBI. The commits should be squashed, though.

@tambeta
Copy link
Author

tambeta commented May 6, 2018

I'm hazy on what LGBI means. As to squashing commits, I'm happy to do that, but I'm not sure what the accepted best practice is on an open PR. Is a forced push overwriting the existing commits with a combined one OK, or should a new PR be opened based off a new branch, the existing PR modified to pull from a new branch, or something else? The Geany hacking document does not specify this.

@elextr
Copy link
Member

elextr commented May 6, 2018

Don't worry, the squash can be done at merge, but the reminder is always good. I take it that upstream ctags accepted the change.

Can you add a test for this? See geany/tests/ctags basically a short piece of ruby and the expected tags.

@codebrainz
Copy link
Member

I'm hazy on what LGBI means.

As far as I can tell, it's Australian for "I looked at the code and it seems OK but I didn't test it, and won't be held responsible if it contains any problems". 😄

@tambeta
Copy link
Author

tambeta commented May 7, 2018

@elextr, upstream did accept the change and the changeset also included a test; I've added it to the PR at hand.

Improves block assignment handling by accepting assignments to sigiled
variables, i.e. those prefixed by `@` or `$` (or really any sequence
thereof).
@elextr
Copy link
Member

elextr commented Jun 17, 2018

@tambeta is 225c4ee in upstream? Improvements are good, but if they are not upstreamed one day someone will pull in the upstream parser and they will all be lost.

@tambeta
Copy link
Author

tambeta commented Jun 18, 2018

True. I have submitted this fix upstream as well.

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