Refactor TMModel line token update and DocumentModelLines #403
Conversation
| public boolean isValid() { | ||
| return !isInvalid; | ||
| } | ||
|
|
||
| public boolean isInvalid() { |
There was a problem hiding this comment.
Should we remove isInvalid then?
| @@ -36,6 +36,10 @@ public List<TMToken> getTokens() { | |||
| return tokens; | |||
| } | |||
|
|
|||
| public boolean isValid() { | |||
There was a problem hiding this comment.
I actually don't like much the name isValid here. Indeed, in such an approach, being "invalid" is semantically precise as "state is not ready", it's actually not the opposite of valid. So if we can avoid adding this method, I think it keeps things clearer for consumers.
There was a problem hiding this comment.
I also thought about this. I think what invalid means is that the contained information is out of date / out of sync with the actual line in the editor. So we could change it to something like isOutdated / isSynced / isUpToDate, etc. any preference?
There was a problem hiding this comment.
We already have isInvalid() which is pretty explicit. Do we need a negated variant at all? If yes, then I thing isUpToDate is the closest meaning.
There was a problem hiding this comment.
I have no hard feelings for either getter. I can remove one. I just found line.isValid() nicer to use than having to use !line.isInvalid() which is a double negation.
There was a problem hiding this comment.
Our software world abuse the "invalid" term so it's not really a negation but a precise description of an actual state unlike "valid" ;) But I'm totally fine with @angelozerr suggestion of renaming isInvalid to isDirty if it helps.
My only concern is more that I don't like isValid.
|
What about isDirty? |
sebthom
force-pushed
the
patch-10
branch
2 times, most recently
from
a24b19f
to
8bd9c51
Compare
|
I decided not to change the name of |
sebthom
changed the title
|
To be honest, I'm now much less familiar with this code than you are. I had a quick look at the general change here and think it's indeed a good one; so if you're confident enough with it and since tests are passing, feel free to merge. |
I refactored/simplified the way the TMModel handles line updates. Still seems to work as expected while needing less LoCs and I believe being easier to follow. Also it now makes use of the new Grammar's timeLimit feature.
Since this is a substantial change of the current logic. It would be great if you could also try it out before merging.