Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Variables structure take two #58

Merged
merged 2 commits into from
Jun 13, 2016

Conversation

noseglid
Copy link
Contributor

@noseglid noseglid commented Jun 13, 2016

Main problem is the identification of variables.
It was too lenient in the first take because it didn't require the a space to exist (e.g. between int and name in int name;).

I am not super happy with the current solution (repeating the "generics, dots, commas blalba" regex and check for a space in between), but I'm at a loss for better alternatives. The way it was before didn't work with generics (see comment in #47). At least this works in all cases.

The issue in #47 is different with this PR. E.g. example (row 13) below gets

  • meta.definition.variable.java storage.type.java on Rectangle2D (correct)
  • meta.definition.variable.java punctuation.separator.period.java on . (correct)
  • meta.definition.variable.java variable.other.property.java on Double (while correct, I'd like to se storage.type.java here too)
  • meta.definition.variable.java variable.other.definition.java on d (correct)

So much better, just a minor wrinkle to iron out.

Example:
screen shot 2016-06-13 at 2 23 17 pm

@winstliu
Copy link
Contributor

Tested this out and couldn't find anything wrong! Thanks once again @noseglid!

@winstliu winstliu merged commit a546e2c into atom:master Jun 13, 2016
@winstliu
Copy link
Contributor

And of course the moment I hit the merge button I see something:

something.add(0, new Integer(((variable >= 0) ? 1 : -1)));

Anything inside Integer() isn't highlighted.

@winstliu
Copy link
Contributor

One more:

else if(testDistanceX < 0 && testDistanceX <= testDistanceY) // Too close to the east/west wall

Everything starting from the D in the second testDistanceX is being tokenized as meta.definition.variable.java.

I really need to test out these PRs more before merging them :/.

@noseglid
Copy link
Contributor Author

I didn't catch that either, and I spent a whole day working with it enabled. I'll look into it.

@noseglid noseglid deleted the variables-rewrite-take-two branch June 14, 2016 05:35
noseglid added a commit to noseglid/autocomplete-java-minus that referenced this pull request Jun 15, 2016
Changed introduced in language-java here:
atom/language-java#58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants