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

problem recognizing chained method call #200

Closed
y opened this Issue Oct 29, 2015 · 55 comments

Comments

Projects
None yet
@y

y commented Oct 29, 2015

at_midnight isn't recognized as a method in the following. The (1) is throwing off the recognition.

$now->plus_days(1)->at_midnight;
@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Oct 29, 2015

Member

Screenshot on 456b3e2:

vim-perl

Member

hoelzro commented Oct 29, 2015

Screenshot on 456b3e2:

vim-perl

@hinrik

This comment has been minimized.

Show comment
Hide comment
@hinrik

hinrik Oct 29, 2015

Contributor

Yeah, chained method calls have never been highlighted. The highlighting also doesn't apply when there's any space in between, even though this is valid in Perl:

method

Contributor

hinrik commented Oct 29, 2015

Yeah, chained method calls have never been highlighted. The highlighting also doesn't apply when there's any space in between, even though this is valid in Perl:

method

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Nov 16, 2015

Member

Is this Not a Bug, then?

Member

hoelzro commented Nov 16, 2015

Is this Not a Bug, then?

@hinrik

This comment has been minimized.

Show comment
Hide comment
@hinrik

hinrik Nov 17, 2015

Contributor

There are some inconsistencies in the highlighting, so I'd say it's a bug. I'll take a look at it soon.

Contributor

hinrik commented Nov 17, 2015

There are some inconsistencies in the highlighting, so I'd say it's a bug. I'll take a look at it soon.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro May 12, 2016

Member

Related: #218

Member

hoelzro commented May 12, 2016

Related: #218

y added a commit to y/vim-perl that referenced this issue May 26, 2017

@y

This comment has been minimized.

Show comment
Hide comment
@y

y May 26, 2017

I took a stab at it and got it to recognize chained method calls and extra whitespace, but it causes some tests to fail. Maybe someone who actually understands vim syntax highlighting can fix it up:
y@7e9dd61

y commented May 26, 2017

I took a stab at it and got it to recognize chained method calls and extra whitespace, but it causes some tests to fail. Maybe someone who actually understands vim syntax highlighting can fix it up:
y@7e9dd61

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Jun 16, 2017

Member

@y Thanks for giving that a try - I'm beginning to question the wisdom of highlighting method calls entirely, though. I would rather not highlight method calls at all (or fix chained method calls) than have a highlighting that works inconsistently. I'm personally leaning towards removing method call highlighting, since 1) I don't personally care for it 2) it's less work to add and maintain proper highlighting if method call highlighting is removed and 3) it will simplify and potentially speed up highlighting. How do you feel about the prospect of removing method call highlighting?

Member

hoelzro commented Jun 16, 2017

@y Thanks for giving that a try - I'm beginning to question the wisdom of highlighting method calls entirely, though. I would rather not highlight method calls at all (or fix chained method calls) than have a highlighting that works inconsistently. I'm personally leaning towards removing method call highlighting, since 1) I don't personally care for it 2) it's less work to add and maintain proper highlighting if method call highlighting is removed and 3) it will simplify and potentially speed up highlighting. How do you feel about the prospect of removing method call highlighting?

@y

This comment has been minimized.

Show comment
Hide comment
@y

y Jun 21, 2017

I'm all for consistency and performance, but as there are hundreds of people directly using the repo and more indirectly as the changes propogate upstream, it's best to get a wider opinion. A blog post on blogs.perl.org would target the right users and get the discussion going. And maybe you'll even get some contributors out of it.

y commented Jun 21, 2017

I'm all for consistency and performance, but as there are hundreds of people directly using the repo and more indirectly as the changes propogate upstream, it's best to get a wider opinion. A blog post on blogs.perl.org would target the right users and get the discussion going. And maybe you'll even get some contributors out of it.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Jun 21, 2017

Member

@y That's a good idea!

Member

hoelzro commented Jun 21, 2017

@y That's a good idea!

hoelzro added a commit that referenced this issue Sep 16, 2017

Remove method highlighting
Experiment for users to try, pertaining to GH #200
@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Sep 16, 2017

Member

Please offer an emoji response to this comment to indicate your vote for how to proceed:

  • 1️⃣ (:one:) for "keep things as they are"
  • 2️⃣ (:two:) for "simplified highlighting"
  • 3️⃣ (:three:) for "fixed, more complicated highlighting"
Member

hoelzro commented Sep 16, 2017

Please offer an emoji response to this comment to indicate your vote for how to proceed:

  • 1️⃣ (:one:) for "keep things as they are"
  • 2️⃣ (:two:) for "simplified highlighting"
  • 3️⃣ (:three:) for "fixed, more complicated highlighting"
@petdance

This comment has been minimized.

Show comment
Hide comment
@petdance

petdance Sep 16, 2017

Contributor

2️⃣

Contributor

petdance commented Sep 16, 2017

2️⃣

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Sep 18, 2017

Member

@mbudde pointed out this bug with my experimental branch: 587b1d0#commitcomment-24367132

Member

hoelzro commented Sep 18, 2017

@mbudde pointed out this bug with my experimental branch: 587b1d0#commitcomment-24367132

@mcsnolte

This comment has been minimized.

Show comment
Hide comment
@mcsnolte

mcsnolte commented Sep 18, 2017

2️⃣

@clonezone

This comment has been minimized.

Show comment
Hide comment
@clonezone

clonezone Sep 18, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

clonezone commented Sep 18, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

@AndyColson

This comment has been minimized.

Show comment
Hide comment
@AndyColson

AndyColson commented Sep 18, 2017

2️⃣

@sjohnston

This comment has been minimized.

Show comment
Hide comment
@sjohnston

sjohnston Sep 18, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

sjohnston commented Sep 18, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

@rollor

This comment has been minimized.

Show comment
Hide comment
@rollor

rollor Sep 18, 2017

3️⃣. Make all complicated options optional and the default case should be indistinguishable from 2️⃣, to the end user.

rollor commented Sep 18, 2017

3️⃣. Make all complicated options optional and the default case should be indistinguishable from 2️⃣, to the end user.

@Smylers

This comment has been minimized.

Show comment
Hide comment
@Smylers

Smylers Sep 18, 2017

2️⃣

I always thought it weird that method names (sometimes) are in the same colour as variable names, not as function names. Or indeed as class methods, which also retain the default colouring.

Stopping trying to colour method names will make this all consistent, which is way more useful in syntax highlighting.

Smylers commented Sep 18, 2017

2️⃣

I always thought it weird that method names (sometimes) are in the same colour as variable names, not as function names. Or indeed as class methods, which also retain the default colouring.

Stopping trying to colour method names will make this all consistent, which is way more useful in syntax highlighting.

@derobert

This comment has been minimized.

Show comment
Hide comment
@derobert

derobert commented Sep 18, 2017

3️⃣

@vadz

This comment has been minimized.

Show comment
Hide comment
@vadz

vadz Sep 18, 2017

2️⃣ but maybe still give a separate class to the arrows to allow highlighting them if desired?

vadz commented Sep 18, 2017

2️⃣ but maybe still give a separate class to the arrows to allow highlighting them if desired?

@gowther-zoho

This comment has been minimized.

Show comment
Hide comment
@gowther-zoho

gowther-zoho Sep 18, 2017

simplified highlighting

I don't feel the emoji.

gowther-zoho commented Sep 18, 2017

simplified highlighting

I don't feel the emoji.

@nfg

This comment has been minimized.

Show comment
Hide comment
@nfg

nfg Sep 18, 2017

2️⃣

nfg commented Sep 18, 2017

2️⃣

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Sep 18, 2017

2️⃣

@neilb

This comment has been minimized.

Show comment
Hide comment
@neilb

neilb commented Sep 18, 2017

2️⃣

@Grinnz

This comment has been minimized.

Show comment
Hide comment
@Grinnz

Grinnz Sep 18, 2017

2️⃣ Don't use vim, but the rules for highlighting a method chain properly would be incredibly complicated given what is possible to give as method arguments.

Grinnz commented Sep 18, 2017

2️⃣ Don't use vim, but the rules for highlighting a method chain properly would be incredibly complicated given what is possible to give as method arguments.

@cees

This comment has been minimized.

Show comment
Hide comment
@cees

cees Sep 19, 2017

2️⃣

cees commented Sep 19, 2017

2️⃣

@ap

This comment has been minimized.

Show comment
Hide comment
@ap

ap Sep 19, 2017

2️⃣

ap commented Sep 19, 2017

2️⃣

@heikojansen

This comment has been minimized.

Show comment
Hide comment
@heikojansen

heikojansen commented Sep 19, 2017

2️⃣

@fffinkel

This comment has been minimized.

Show comment
Hide comment
@fffinkel

fffinkel commented Sep 19, 2017

2️⃣

@trwyant

This comment has been minimized.

Show comment
Hide comment
@trwyant

trwyant Sep 19, 2017

2️⃣ Why highlight a method chain of any length differently than a subroutine call?

trwyant commented Sep 19, 2017

2️⃣ Why highlight a method chain of any length differently than a subroutine call?

@mmcclimon

This comment has been minimized.

Show comment
Hide comment
@mmcclimon

mmcclimon commented Sep 19, 2017

3️⃣

@batlock666

This comment has been minimized.

Show comment
Hide comment
@batlock666

batlock666 commented Sep 20, 2017

2️⃣

@merrilymeredith

This comment has been minimized.

Show comment
Hide comment
@merrilymeredith

merrilymeredith Sep 20, 2017

2️⃣

I just want it consistent, and the current highlighting also breaks if you have newlines in your method chain to look this way after the newline anyway. Performance and maintenance also A-OK!

merrilymeredith commented Sep 20, 2017

2️⃣

I just want it consistent, and the current highlighting also breaks if you have newlines in your method chain to look this way after the newline anyway. Performance and maintenance also A-OK!

@rba

This comment has been minimized.

Show comment
Hide comment
@rba

rba Sep 20, 2017

2️⃣

rba commented Sep 20, 2017

2️⃣

@Camspi

This comment has been minimized.

Show comment
Hide comment
@Camspi

Camspi commented Sep 21, 2017

2️⃣

@mannih

This comment has been minimized.

Show comment
Hide comment
@mannih

mannih commented Sep 22, 2017

2️⃣

@borisdaeppen

This comment has been minimized.

Show comment
Hide comment
@borisdaeppen

borisdaeppen Sep 22, 2017

2️⃣ because consistency, if 3️⃣ has the drawbacks mentioned

borisdaeppen commented Sep 22, 2017

2️⃣ because consistency, if 3️⃣ has the drawbacks mentioned

@mikkoi

This comment has been minimized.

Show comment
Hide comment
@mikkoi

mikkoi commented Sep 22, 2017

2️⃣

@simongreen-net

This comment has been minimized.

Show comment
Hide comment
@simongreen-net

simongreen-net commented Sep 25, 2017

(2️⃣)

@chsanch

This comment has been minimized.

Show comment
Hide comment
@chsanch

chsanch commented Sep 25, 2017

2️⃣

@mrallen1

This comment has been minimized.

Show comment
Hide comment
@mrallen1

mrallen1 commented Sep 25, 2017

2️⃣

@dnebauer

This comment has been minimized.

Show comment
Hide comment
@dnebauer

dnebauer commented Sep 25, 2017

2️⃣

@abraxxa

This comment has been minimized.

Show comment
Hide comment
@abraxxa

abraxxa Sep 25, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

abraxxa commented Sep 25, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Sep 25, 2017

Member

Ok, it's been a week now - it looks like the "simplified highlighting" folks have it. We can start working on making the syntax highlighting simpler and more consistent soon.

Member

hoelzro commented Sep 25, 2017

Ok, it's been a week now - it looks like the "simplified highlighting" folks have it. We can start working on making the syntax highlighting simpler and more consistent soon.

@jwittkoski

This comment has been minimized.

Show comment
Hide comment
@jwittkoski

jwittkoski Sep 25, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

jwittkoski commented Sep 25, 2017

3️⃣, where arrows and parens get a different syntax class than the method name.

@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Sep 25, 2017

Member

That being said, I would look at a PR implementing 3️⃣ behind an opt-in option, as suggested here.

Member

hoelzro commented Sep 25, 2017

That being said, I would look at a PR implementing 3️⃣ behind an opt-in option, as suggested here.

@ivanwills

This comment has been minimized.

Show comment
Hide comment
@ivanwills

ivanwills Sep 25, 2017

2️⃣ It is better to be simple, clean and fast

ivanwills commented Sep 25, 2017

2️⃣ It is better to be simple, clean and fast

@chazubell

This comment has been minimized.

Show comment
Hide comment
@chazubell

chazubell commented Sep 25, 2017

image

@sverrewl

This comment has been minimized.

Show comment
Hide comment
@sverrewl

sverrewl commented Sep 25, 2017

2️⃣

@bewuethr

This comment has been minimized.

Show comment
Hide comment
@bewuethr

bewuethr commented Sep 26, 2017

3️⃣

@elohmrow

This comment has been minimized.

Show comment
Hide comment
@elohmrow

elohmrow commented Sep 26, 2017

2️⃣

@mishagale

This comment has been minimized.

Show comment
Hide comment
@mishagale

mishagale commented Sep 26, 2017

2️⃣

@gs230937207

This comment has been minimized.

Show comment
Hide comment
@gs230937207

gs230937207 commented Sep 27, 2017

2️⃣

@gwadej

This comment has been minimized.

Show comment
Hide comment
@gwadej

gwadej commented Sep 27, 2017

2️⃣

hoelzro added a commit that referenced this issue Oct 27, 2017

Remove method highlighting
Experiment for users to try, pertaining to GH #200
@hoelzro

This comment has been minimized.

Show comment
Hide comment
@hoelzro

hoelzro Oct 27, 2017

Member

Ok, I've implemented 2️⃣ and pushed that into the dev branch. If you run into any issues with the change, please file a separate isssue. If anyone would like to take a crack at implementing 3️⃣, feel free to reach out!

Member

hoelzro commented Oct 27, 2017

Ok, I've implemented 2️⃣ and pushed that into the dev branch. If you run into any issues with the change, please file a separate isssue. If anyone would like to take a crack at implementing 3️⃣, feel free to reach out!

@hoelzro hoelzro closed this Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment