-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
…hlighter_check_path() in order to allow edit in the middle of the line without losing path colors in some cases
d74a57e
to
6d9a340
Compare
…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).
@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 |
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.
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?
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
Supporting Thanks again for your contributions! |
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
is what I did to isolate ece762e for testing issue 165. Another option is to use an interactive rebase:
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. |
I see your point. Sounds logical at first, but that's not so simple. Let me propose other, simpler solution. 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. |
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 Does this make sense?
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.
Thanks for the information. Let's discuss the LS_COLORS change further on that separate "Add LS_COLORS highlighting" issue from the first paragrah ☺
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 |
Daniel, all bug fixes and improvements are described in the commits messages.
The PR was opened one year ago, and 26 commits were added during last year consecutively.
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. |
…ighlighters/main/README.md.
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.
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.
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.)
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 |
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. |
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. 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?! |
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 |
Not that I know of.
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. |
…on-notice-v1 README: Clarify which projects are deprecated and which aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
I am also interested in incorporating the work done in |
(Closing/reopening to trigger continuous integration) |
@nicoulaj |
Any reason this hasn't been solved yet, 5 years later? If I type It seems like #244 was also attempting to solve it, but has not been merged. Additionally, the LS_COLORS features would be nice too |
It hasn't been implemented because no one has implemented it. If you'd like to help, that would be welcome. |
I've added a few things, mostly to highlighters/main/main-highlighter.zsh:
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
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.
This is minor improvement to guarantee that path prefix can be with colors even if edit is made in the middle of the line.
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.