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

strip trailing dots in goto/datatip (and rename) #208

Merged
merged 7 commits into from
Nov 1, 2019
Merged

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Oct 29, 2019

While implementing #203, I had to implement the logic to remove trailing dots and found that this is better to be merged as a separate feature, since the logic can be used for goto/datatip.

In short, if we have a word like

Atom.SYMBOLSCACHE.age

and then, we can get goto and datatip for:

  • Atom when our cursor is anywhere on Atom
  • Atom.SYMBOLSCACHE when our cursor is anywhere on SYMBOLSCACHE
  • no datatip if our cursor is on age (the field)

(c.f.: The current behaviour: we can't get any goto and datatip at all)


Requires JunoLab/atom-julia-client#651

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #208 into master will increase coverage by 0.19%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   40.03%   40.22%   +0.19%     
==========================================
  Files          40       40              
  Lines        1856     1857       +1     
==========================================
+ Hits          743      747       +4     
+ Misses       1113     1110       -3
Impacted Files Coverage Δ
src/utils.jl 86.2% <ø> (ø) ⬆️
src/display/methods.jl 86.66% <100%> (ø) ⬆️
src/goto.jl 81.81% <100%> (+1.02%) ⬆️
src/datatip.jl 79.68% <90.9%> (+3.32%) ⬆️

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 66270cc...9d80e2f. Read the comment docs.

@aviatesk aviatesk mentioned this pull request Oct 29, 2019
14 tasks
@aviatesk aviatesk force-pushed the avi/partialdots branch 2 times, most recently from 35d8f31 to 9759a5a Compare October 29, 2019 11:12
@aviatesk
Copy link
Member Author

@pfitzseb any objection on this idea ? I will make changes so that the striping would be done in the frontend, though.

@pfitzseb
Copy link
Member

Makes sense to me, I think. Might be somewhat unintuitive, but we can probably add some clarity by highlighting the part we're using (if that makes sense).

@aviatesk aviatesk force-pushed the avi/partialdots branch 3 times, most recently from e43804d to 5d8b703 Compare October 30, 2019 19:34
@aviatesk
Copy link
Member Author

okay, so the striping task is now all done in the frontend (basically by using different regex for the beginning and end of the word), and the awkward fullword stuff here are all removed.

Might be somewhat unintuitive

yes, maybe especially when we get goto/datatip for Mod when a cursor is on Mod.foo or something.
(But at least there are corresponding underline/highlights)


Still I'm not sure which behaviour do people prefer (not so many of users mayn't even care about the difference tho)

@aviatesk
Copy link
Member Author

aviatesk commented Nov 1, 2019

The last two commits refactor the logic in gotoglobalsymbols and update tests according to that.

Once the CI passes, I will merge this PR, since now trailing dots are stripped all in the frontend (JunoLab/atom-julia-client#651) and the changes in this PR only do the trivial refactors.

If we find the stripping trailing dots really unintuitive, we can discard/revert JunoLab/atom-julia-client#651, but the changes in this PR wouldn't get affected by that at all.

@aviatesk aviatesk merged commit d7af2d5 into master Nov 1, 2019
@aviatesk aviatesk deleted the avi/partialdots branch November 1, 2019 09:53
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.

2 participants