Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Conversation

@ijsnow
Copy link
Contributor

@ijsnow ijsnow commented Aug 13, 2018

No description provided.

@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #38 into master will increase coverage by 1.11%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   64.13%   65.24%   +1.11%     
==========================================
  Files          15       15              
  Lines         566      587      +21     
  Branches      146      155       +9     
==========================================
+ Hits          363      383      +20     
- Misses        203      204       +1
Impacted Files Coverage Δ
src/hoverifier.ts 61.98% <78.57%> (+3.98%) ⬆️
src/helpers.ts 56.41% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 849914f...f037f1c. Read the comment docs.

@ijsnow ijsnow changed the title WIP DONT MERGE: Position adjuster feat: add optional position adjuster to HoverifierOptions Aug 13, 2018
@ijsnow ijsnow requested a review from chrismwendt August 13, 2018 22:23
Copy link
Contributor

@chrismwendt chrismwendt left a comment

Choose a reason for hiding this comment

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

Just a couple nitpicks

* that the effected positions have actually been adjusted as intended but this is impossible with the current implementation. We can assert that the `HoverFetcher` and `JumpURLFetcher`s
* have the adjusted positions (AdjustmentDirection.CodeViewToActual). However, we cannot reliably assert that the code "highlighting" the token has the position adjusted (AdjustmentDirection.ActualToCodeView).
*/
it('PositionAdjuster gets called when expecteds', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

"expected"?


const inputDiagram = 'ab'
// There is probably a bug in code that is unrelated to this feature that is causing the PositionAdjuster to be called an extra time.
const outputDiagram = 'a(ba)'
Copy link
Contributor

Choose a reason for hiding this comment

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

What should this diagram look like if there were no bug, or is there an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add to the comment, but it should be -(ba). Since its a(ba), and a is the direction that adjustPosition gets called with when we're getting the position the token is in the dom(not vcs), I'm assuming that we're calling the highlight logic immediately and then the two expected calls are getting called on the next tick.

}))
)
: of({ adjustPosition, codeView, resolveContext, position, ...rest })
),
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like map could be used in place of switchMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjustPosition returns an observable so I need to use switchMap to flatten it into just its output.

@ijsnow ijsnow merged commit 178b32e into master Aug 14, 2018
@ijsnow ijsnow deleted the position-adjuster-1 branch August 14, 2018 16:05
@sourcegraph-bot
Copy link

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants