Skip to content
This repository was archived by the owner on Mar 23, 2024. It is now read-only.

Conversation

@mdevils
Copy link
Member

@mdevils mdevils commented Jul 31, 2014

This is a the start of 2.x work. We can significantly simplify new rules using this kind of navigation.

@mdevils
Copy link
Member Author

mdevils commented Jul 31, 2014

/cc @mikesherov @markelog

@mdevils mdevils added this to the 2.0 milestone Jul 31, 2014
@mdevils
Copy link
Member Author

mdevils commented Aug 7, 2014

@mikesherov Can you take a look please, so I can continue to work on 2.0?

@markelog
Copy link
Member

markelog commented Aug 7, 2014

as discussed, i would add tests for new methods, we probably could start using some coverage tool for it too :-)

@mikesherov
Copy link
Contributor

Agreed with @markelog, if we're going to land these new methods, tests and an actual implementation in at least one rule would be nice. Other than that I have a few comments I'll leave.

lib/js-file.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

findPrevToken

@markelog
Copy link
Member

markelog commented Aug 7, 2014

actual implementation in at least one rule would be nice

@mdevils already find use of them – 6733799

@mikesherov
Copy link
Contributor

@mdevils already find use of them – 6733799

Yes, but perhaps land them them with this PR?

@markelog
Copy link
Member

markelog commented Aug 7, 2014

Yes, but perhaps land them them with this PR?

That is the idea

@mdevils mdevils force-pushed the mdevils-2.x-navigation branch 3 times, most recently from d8dcd44 to 55e2a45 Compare August 26, 2014 11:04
@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 55e2a45 on mdevils-2.x-navigation into 0f281cf on 2.x.

@mdevils
Copy link
Member Author

mdevils commented Aug 26, 2014

  • Integrated coverage: Coverage integration #557
  • Renamed methods: findTokenBackward, findTokenForward -> findPrevToken, findNextToken.
  • Wrote tests for all new methods.

@mikesherov
Copy link
Contributor

👍 from me. This looks good to land for 2.0

@markelog
Copy link
Member

Same here

@mdevils mdevils force-pushed the mdevils-2.x-navigation branch from 55e2a45 to ba562c4 Compare August 27, 2014 10:27
@mdevils mdevils merged commit ba562c4 into 2.x Aug 27, 2014
@mdevils mdevils deleted the mdevils-2.x-navigation branch August 27, 2014 10:28
@markelog
Copy link
Member

I wonder if we can merge this to master branch, it seems we could already start using it

@mdevils
Copy link
Member Author

mdevils commented Aug 27, 2014

Well, we can.

Copy link
Member

Choose a reason for hiding this comment

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

So if that returns undefined, than customer of that method should check for it before using? So i can't just do if (getTokenByRangeStart(...).value === '['), i would need to do something like this:

var token = getTokenByRangeStart(...);

if (token && token.value === "["){
...
}

Maybe, for convenience sake, it could return some fake token instead of undefined, like

{ 
  type: '',
  value: '',
  range: [ -1, -1 ],
  loc: { start: { line: -1, column: -0 }, end: { line: -1, column: -1 } },
  _tokenIndex: -1 
}

Copy link
Member

Choose a reason for hiding this comment

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

type: 'fake' :-) agreed with fake token, but probably we should be more concrete on this.

Copy link
Member

Choose a reason for hiding this comment

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

This token isn't there, so it's not really fake, such situations might exist if current token is first or last token in the tree

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I'm not sure there can be situation with no token (if you pass real node range).

Copy link
Member

Choose a reason for hiding this comment

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

@mdevils that is mostly for the getNextToken/getPrevToken, here, just for the consistency

@markelog
Copy link
Member

Also, how about iteratePunctuatorsByName, would ease those rules that have to check through tokens?

Has a downside, since it will reflect on performance.

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.

6 participants