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

colors for files, command_prefix + more minor modifications #152

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

jimmijj
Copy link

@jimmijj jimmijj commented Sep 26, 2014

I've added a few things, mostly to highlighters/main/main-highlighter.zsh:

  1. Highlighting for files based of LS_COLORS environment variable with the following features:
    a) both 8 and 256 color terminal are supported
    b) attributes (ex, ln, st...) and extension of file are taken into account
    c) file prefix is recognized and properly colored too
    d) does not interfere with path style (different colors)
    e) possibility to edit filename in the middle of the line without loosing colors
    f) user can override color for one particular file type or all at once in .zshrc
  2. command_prefix
    This should solve (at least partially) issue Highlighting completion state #148. The problem is that if path_approx style is enabled and user run command in $HOME directory then one of many dot-files in this folder match almost all commands. To solve this function check_command should be run before check_path. By default I set this style to the same as command style, this probably what user is expecting.
  3. Changed ${#BUFFER} -> ${#LBUFFER} in path_check
    This is minor improvement to guarantee that path prefix can be with colors even if edit is made in the middle of the line.
  4. missing separators &|, |& and &!
    It seems obvious to include all command separators to the list, but for some reason these three were not there. I see this also in user 'm0vie' commit, but is was not merged to main tree so far.

Additionally I've described new file-highlighting under ./highlighters/main/README.md and added small screenshot to ./README.md to give new users a hint what to expect from zsh-syntax-highlighting package.

Comments and questions are welcome.

@jimmijj jimmijj closed this Sep 26, 2014
@jimmijj jimmijj reopened this Sep 26, 2014
@jimmijj jimmijj changed the title colors for files, command_prefix + more minor modification colors for files, command_prefix + more minor modifications Sep 26, 2014
@jimmijj jimmijj force-pushed the master branch 2 times, most recently from d74a57e to 6d9a340 Compare October 3, 2014 11:38
…ghlight properly consecutive command separators like 'echo a; ; ; echo b'.
…3, otherwise it matches too many things. (but do we need path_approx at all?)
…fted by one character starting from second line. This behaviour was due to splitting of BUFFER using shell parser ${(z)BUFFER}, which basically changes all newlines to semicolons.
…ighlighter when cursor has moved. Normally turning on this feature for the whole main highlighter is not advisable, however it is still helpful in edge cases and solves the problem with highlighting the prefix of the path and file. To prevent slowdown the predicate_switcher is defined in such a way that it activates main highlighter with respect to cursor movement just for one call, and after that returns automatically to the default mode, i.e. highlighting only after buffer is modified.
…s solves issue that prefix '/l' matches '/bin//ls' (with two slashes what is valid syntax for zsh).
@jimmijj
Copy link
Author

jimmijj commented Sep 4, 2015

@danielshahaf Please note that I've submitted PR one year ago! There was no response whatsoever and I committed a lot of patches since then, so I'm not sure how to split them right now. Additionally I see that in last two days you merged something, so it is possible there are some conflicts with my work.

To give you brief overview: generally I fixed few bugs and added some features, but main work (in number of code lines and least) focused on adding highlighting of files as in ls command. Please read log from my commits to learn more and let me know what is your idea of merging all of that. I could in principle open pull request for all commits separately (if this is possible in git), but as I said - there could be conflicts right now.

@danielshahaf
Copy link
Member

@danielshahaf Please note that I've submitted PR one year ago! There was no response whatsoever

Yes, you're not alone. I also submitted several PRs over the last year and got no response to them, too. But a few days ago I've been granted push access, and I've been going through the issue backlog.

and I committed a lot of patches since then, so I'm not sure how to split them right now.

It's possible to split them, but let's not worry about the mechanics of preparing an individual feature branch for each change right now. What I would like to have right now is a separate issue for each logical change, so we can discuss them individually. Those issues that come to fruition can then be promoted to pull requests (and we can worry about the mechanics at that point). Does that make sense?

Additionally I see that in last two days you merged something, so it is possible there are some conflicts with my work.

The github UI indicates that there are conflicts. There are indeed semantic conflicts, for example, jimmijj@df99f5f was fixed in a different way by #159. (Which I've already merged, not having seen your patch to that same issue beforehand.) On the other hand, I think your branch fixes a few outstanding issues; for example, jimmijj@ece762e might fix #165, and jimmijj@3a6ba00 might fix #40.

Actually, I've gone ahead and tested it, and jimmijj@ece762e does fix issue #165 for the special case where $zle_highlight explicitly contains a region:… element. I'll comment further on that issue.

To give you brief overview: generally I fixed few bugs and added some features, but main work (in number of code lines and least) focused on adding highlighting of files as in ls command. Please read log from my commits to learn more and let me know what is your idea of merging all of that. I could in principle open pull request for all commits separately (if this is possible in git), but as I said - there could be conflicts right now.

Supporting LS_COLORS-like highlighting for filenames (i.e., not just default to underline)? This does sound like an interesting enhancement. However, to be honest, I only have push access here for a week, and I don't have yet a good gut feeling for judging suggested features. So I'd rather start with the bugfixes than with new features.

Thanks again for your contributions!

@danielshahaf
Copy link
Member

It's possible to split them, but let's not worry about the mechanics of preparing an individual feature branch for each change right now. What I would like to have right now is a separate issue for each logical change, so we can discuss them individually. Those issues that come to fruition can then be promoted to pull requests (and we can worry about the mechanics at that point). Does that make sense?

To break out a few related commits as their own branch, in general, you would open a new branch off upstream/master and then use git cherry-pick to merge individual commits from the jimmijj/master branch; for example:

git fetch upstream jimmijj
git checkout -b i165-v1 upstream/master
git cherry-pick ece762e81798bc4448bf17f68a4792d1117dc032

is what I did to isolate ece762e for testing issue 165. Another option is to use an interactive rebase:

git fetch upstream jimmijj
git checkout -b i165-v1 jimmijj/master        # note different startpoint
git rebase --interactive upstream/master
# in the interactive editor, delete everything but the revisions that should be
# part of the target branch

But, again, I don't want to ask you to do a lot of work; that's why I asked for issues first, to discuss each proposed feature on, before making the further effort of breaking out individual pull requests.

@jimmijj
Copy link
Author

jimmijj commented Sep 5, 2015

What I would like to have right now is a separate issue for each logical change, so we can discuss them individually.

I see your point. Sounds logical at first, but that's not so simple.
The problem is that I cannot isolate n_th_ commit alone - in many cases there are conflicts without commit n-2, while in n-1 I changed other (not related with n-1) thing, but in the same part of code. In other words there are conflicts with my own work if one do not take into account previous commits. It is huge amount of work to disentangle all "logical changes" from my commits right now.

Let me propose other, simpler solution.
As far as I can see you pushed just a few commits so far, all of them with not much code inside. I would suggest to revert them -> push my commits -> add previously reverted commits back -> solve conflicts. The point is that it is easier to resolve conflicts in few lines of code and a few commits. You may dedicate separate branch for the whole operation, and merge this branch with master only when everything will be cleaned. I can help you with all of that if you are willing.

Oh, and concerning files highlighting, there is option to switch it off, or set to underline, so one may set highlighting behaviour in the same fashion as without this change. It may be even default. Please check main/README.md where I described the procedure.

BTW, I have other local changes, and further ideas, but I will not commit anything until we decide what to do with stuff which is already present.

@danielshahaf
Copy link
Member

I see your point. Sounds logical at first, but that's not so simple.
The problem is that I cannot isolate nth commit alone - in many cases there are conflicts without commit n-2, while in n-1 I changed other (not related with n-1) thing, but in the same part of code. In other words there are conflicts with my own work if one do not take into account previous commits. It is huge amount of work to disentangle all "logical changes" from my commits right now.

I am not asking you to disentangle the conflicts and interdependencies. I am only asking you to enumerate the high-level logical changes and open a separate issue for each logical change. The issues might be "Add screenshot", "Add LS_COLORS support", "Add redirection operators", and so on. Each such issue issue should simply name the logical change and indicate which commits on your existing branch are relevant to it.

In other words, I am simply asking you to describe the bugs you've fixed and features you've added. The only git command you'll need for this is git log.

Does this make sense?

Let me propose other, simpler solution.
As far as I can see you pushed just a few commits so far, all of them with not much code inside. I would suggest to revert them -> push my commits -> add previously reverted commits back -> solve conflicts. The point is that it is easier to resolve conflicts in few lines of code and a few commits. You may dedicate separate branch for the whole operation, and merge this branch with master only when everything will be cleaned. I can help you with all of that if you are willing.

I am not going to rewind master, to throw away development work that has been done (either by me or by you), or to import code en bloc. Neither of these would be good practice.

The right way forward is to continue making small, incremental changes.

It is unfortunate that you developed multiple features straddling 26 commits without separating them from each other or communicating with upstream (I don't see any PRs from you before this one). That is why we have to split your work now in order to integrate it. However, and that is the point, the right way forward is to split your branch into logical features, and merge each feature separately.

That means each logical feature will need to be prepared as a feature branch, implementing only that feature, prior to being merged. If you want, I can help with preparing the feature branches, and both of us will review the feature branches prior to merging them back upstream.

Oh, and concerning files highlighting, there is option to switch it off, or set to underline, so one may set highlighting behaviour in the same fashion as without this change. It may be even default. Please check main/README.md where I described the procedure.

Thanks for the information. Let's discuss the LS_COLORS change further on that separate "Add LS_COLORS highlighting" issue from the first paragrah ☺

BTW, I have other local changes, and further ideas, but I will not commit anything until we decide what to do with stuff which is already present.

Thanks for holding back on those ideas; that should make both of our lives easier. Are those local changes and further ideas independent of the work you have done so far, or do they build on top of it?

Cheers,

Daniel

@jimmijj
Copy link
Author

jimmijj commented Sep 6, 2015

I am not asking you to disentangle the conflicts and interdependencies. I am only asking you to
enumerate the high-level logical changes and open a separate issue for each logical change. The
issues might be "Add screenshot", "Add LS_COLORS support", "Add redirection operators", and so
on. Each such issue issue should simply name the logical change and indicate which commits on
your > existing branch are relevant to it.

Daniel, all bug fixes and improvements are described in the commits messages.
What's the point of copying that list to the issue tracker???
Those bugs have been already solved, so one might at most mark already existing issues as solved.

I am not going to rewind master, to throw away development work that has been done (either
by me > or by you), or to import code en bloc. Neither of these would be good practice.
The right way forward is to continue making small, incremental changes.
It is unfortunate that you developed multiple features straddling 26 commits without separating
them from each other or communicating with upstream (I don't see any PRs from you before this
one)

The PR was opened one year ago, and 26 commits were added during last year consecutively.
Last commit (26th) was added 2 days ago. It is not my false that upstream didn't respond for very long time and when finally did conflicted patches were pulled randomly instead of first think how to solve 1 year of negligence and conciliate awaiting PRs.

That is why we have to split your work now in order to integrate it. However, and that is the point,
the right way forward is to split your branch into logical features, and merge each feature
separately.
That means each logical feature will need to be prepared as a feature branch, implementing only
that feature, prior to being merged. If you want, I can help with preparing the feature branches, and > both of us will review the feature branches prior to merging them back upstream.

I'm sorry, but I'm not willing to prepare separate branches for each feature which was already implemented, that is completely useless work.

I don't want to go backwards, and in my opinion current upstream branch with many bugs and missing features, is just obsolete w.r.t. my branch. I see that you don't want to merge it so perhaps it is better if I will just continue my work separately.

@danielshahaf
Copy link
Member

I am not asking you to disentangle the conflicts and interdependencies. I am only asking you to
enumerate the high-level logical changes and open a separate issue for each logical change. The
issues might be "Add screenshot", "Add LS_COLORS support", "Add redirection operators", and so
on. Each such issue issue should simply name the logical change and indicate which commits on
your > existing branch are relevant to it.

Daniel, all bug fixes and improvements are described in the commits messages.
What's the point of copying that list to the issue tracker???

  1. Being able to discuss and review each change individually.
  2. Minimize deviations from normal development process. (Deviations lead to bugs. This is true not only in software development but also in other disciplines.)

Those bugs have been already solved, so one might at most mark already existing issues as solved.

I am not going to rewind master, to throw away development work that has been done (either
by me > or by you), or to import code en bloc. Neither of these would be good practice.
The right way forward is to continue making small, incremental changes.
It is unfortunate that you developed multiple features straddling 26 commits without separating
them from each other or communicating with upstream (I don't see any PRs from you before this
one)

The PR was opened one year ago, and 26 commits were added during last year consecutively.

There seems to be a misunderstanding: I had the impression that you pushed all 26 commits on one day, but you report you added those 26 commits "consecutively". My impression was based on the github UI (which indicates all commits were added on 26 Sep 2014), and is to a certain degree corroborated by the git metadata (which indicates 21 of the commits were authored on or before Oct 2014, but doesn't indicate when each commit was pushed to the PR's branch). In any case, I apologise for the misunderstanding.

Last commit (26th) was added 2 days ago. It is not my false that upstream didn't respond for very long time and when finally did conflicted patches were pulled randomly instead of first think how to solve 1 year of negligence and conciliate awaiting PRs.

I did go through the outstanding PRs when I got push access. Your PR had 24 patches on it that implemented about 5 orthogonal features and bugfixes, so I mentally labeled it as "cannot be merged immediately" and moved on. (I realize this might sound harsh, but such is the nature of triage.)

If you had authored 5 different PRs each implementing a single bugfix or feature, I would probably have merged 3 or 4 of them in my first sweep through the outstanding PRs, before even merging my own fixes, and no action on your part would have been required or requested. But your PR was not split, so I decided to give priority to others.

I also decided to give priority to bugfixes over new features, by the way.

That is why we have to split your work now in order to integrate it. However, and that is the point,
the right way forward is to split your branch into logical features, and merge each feature
separately.
That means each logical feature will need to be prepared as a feature branch, implementing only
that feature, prior to being merged. If you want, I can help with preparing the feature branches, and > both of us will review the feature branches prior to merging them back upstream.

I'm sorry, but I'm not willing to prepare separate branches for each feature which was already implemented, that is completely useless work.

I did not ask you to make feature branches.

As to whether it is "useless work", that is somewhat subjective. Splitting your work will allow it to be merged upstream, and having your work merged upstream is as much in your interest as in ours. (In your case, it's your interest to reduce the divergence so you can reuse more of the code that gets added to upstream.)

I don't want to go backwards, and in my opinion current upstream branch with many bugs and missing features, is just obsolete w.r.t. my branch. I see that you don't want to merge it so perhaps it is better if I will just continue my work separately.

Like I said, I don't want to merge it en bloc, but I would be happy to merge all of it, one feature at a time. (I hope this process does not seem onerous to you. It is the standard process that nearly all open-source projects use.)

I would still like to merge your work upstream. You do not have to help with this if you don't want to, but I hope we do have your blessing to merge your work into this repository, even if we do all the splitting and conflicts-resolving legwork that's now required.

And, in any case, I will welcome any pull requests you might choose to send in the future.

Cheers,

Daniel

@danielshahaf
Copy link
Member

To summarize: I'd like to work with you. I'd like to start by merging to this repository the bugfixes you have on your branch. I'm happy to do the splitting and merging work myself.

@blueyed
Copy link

blueyed commented Sep 7, 2015

FWIW, I agree with @danielshahaf that it would have been better to create separate branches (and PRs) for each bugfix/feature. But I can also see that it is convenient to just work your own "master" branch instead - especially given the stale upstream back then.
(Some recommendations in this regard: https://github.com/github/hub/ is really nice to interact with Github; and you can use a local branch, e.g. "local" for integration of your own PRs/branches).

Given the latest update to the README from @jimmijj's fork, it seems like the way forward appears to be cherry-picking stuff from this branch/PR manually?!

@derimagia
Copy link

Has their been any work in getting LS_COLORS moved in? https://github.com/trapd00r/zsh-syntax-highlighting-filetypes is an old project for doing this that was forked from this project. I can take a look at cleaning this part out and opening a separate pr if no one is making the effort yet

@danielshahaf
Copy link
Member

Has their been any work in getting LS_COLORS moved in?

Not that I know of.

https://github.com/trapd00r/zsh-syntax-highlighting-filetypes is an old project for doing this that was forked from this project. I can take a look at cleaning this part out and opening a separate pr if no one is making the effort yet

A separate PR would be the way to go, yes. Feel free to open a non-PR issue first to discuss things before moving on to the implementation phase.

Copy link

@melMass melMass left a comment

Choose a reason for hiding this comment

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

+1

@diego898
Copy link

I am also interested in incorporating the work done in https://github.com/trapd00r/zsh-syntax-highlighting-filetypes and in the above PR!

@nicoulaj nicoulaj closed this Aug 15, 2017
@nicoulaj nicoulaj reopened this Aug 15, 2017
@nicoulaj
Copy link
Member

(Closing/reopening to trigger continuous integration)

@blueyed
Copy link

blueyed commented Aug 15, 2017

@nicoulaj
Needs conflicts to be resolved first.

@vegerot
Copy link

vegerot commented Sep 9, 2019

Any reason this hasn't been solved yet, 5 years later? If I type ifconfi the text is red, and only turns green when I type ifconfig).

It seems like #244 was also attempting to solve it, but has not been merged.

Additionally, the LS_COLORS features would be nice too

@danielshahaf
Copy link
Member

It hasn't been implemented because no one has implemented it. If you'd like to help, that would be welcome.

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

Successfully merging this pull request may close these issues.

None yet

8 participants