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

[RFC] Add optional third tab character in "listchars" #3565

Closed
wants to merge 2 commits into from
Closed

[RFC] Add optional third tab character in "listchars" #3565

wants to merge 2 commits into from

Conversation

natbraun
Copy link

@natbraun natbraun commented Oct 30, 2015

This PR is a rebase of #3088

@fmoralesc
Copy link
Contributor

It seems like linting errors must be fixed, and a test would be great. (It seems like I closed the previous PR just because I got lazy :p)

@natbraun
Copy link
Author

@fmoralesc Thanks! However, I don't understand the clint errors, I can't relate the lines to the code.
I'd like to write a test, could you please describe the procedure to add one? Thanks!

@fmoralesc
Copy link
Contributor

@natbraun Sometimes the lint errors come from lines around the changes you made, but I see what you mean, the line offsets are wrong for some reason. For example, the error in line 2779 in screen.c seems to happen in 2783 instead:

+           c_final = NUL;
+          }
+          else {
             c_extra = 

I suggest you try identifying finding the kind of errors lint identified in the region surrounding the changes you made.

Now, to add a test I think the best probably is to try to expand some existing test for listchars (functional/legacy/listchars_spec.lua), guiding yourself by the existing tests should help you understand how that is supposed to work.

@natbraun
Copy link
Author

@fmoralesc Just added some tests. Thanks for your feedback :)

@fmoralesc
Copy link
Contributor

@natbraun Great! Now, the only issue is the lint errors. I'll try to take a look, but no promises ;) (so feel free to go ahead)

@natbraun
Copy link
Author

@fmoralesc Thanks a lot for your feedback! Really appreciate it :) Now, I gave a look at the lint errors, but to be honest I don't see where the errors come from... I'll take a closer look now.

@natbraun
Copy link
Author

@fmoralesc OK, I'm starting to see clearer:

  1. src/nvim/option.c:3358: If an else has a brace on one side, it should have it on both: this is true, but it's not been introduced by my code.
  2. Same goes for FALSE & lin elength

I can provide a fix if needed. Thanks.

@fmoralesc
Copy link
Contributor

@natbraun As I mentioned earlier, sometimes the linter looks for errors in lines surrounding the changes made. At any rate, the travis build must pass so this can be merged (I think), so if you can, please fix those errors (and any others that might arise).

@natbraun
Copy link
Author

@fmoralesc Looks better now :)

n_extra = get_breakindent_win(wp, ml_get_buf(wp->w_buffer, lnum, FALSE));
c_final = NUL;
n_extra = get_breakindent_win(wp,
ml_get_buf(wp->w_buffer, lnum, false));
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be indented under 'w' in 'wp':

+          n_extra = get_breakindent_win(wp,
+                                        ml_get_buf(wp->w_buffer, lnum, false));

but since that brings the issue of line length, perhaps indenting 2 spaces after get_breakindent_win might be better?

+          n_extra = get_breakindent_win(wp,
+                      ml_get_buf(wp->w_buffer, lnum, false));

Either way, no big deal.

EDIT: I made some mistakes on this. Anyway: my suggestion is not "in-style" either, it just seems less awkward than splitting this line like this:

+          n_extra = get_breakindent_win(wp,
+                                        ml_get_buf(wp->w_buffer,
+                                                   lnum, false));

Copy link
Author

Choose a reason for hiding this comment

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

OK, fixed using the first solution: it actually fits!

@fmoralesc
Copy link
Contributor

Sure does, and the errors in QB seem to be unrelated. Great work! I made some minor comments.

@justinmk justinmk added this to the 0.2 milestone Oct 31, 2015
@natbraun
Copy link
Author

Thanks for your feedback @fmoralesc ! Just pushed an updated & rebased PR.

@fmoralesc
Copy link
Contributor

Thanks, @natbraun. It looks good to me, but let's wait for a second opinion. @justinmk marked this for 0.2 (this just means that it might land after 0.1 is released, so it shouldn't divert people from working on the more urgent issues), but it might be merged earlier depending on someone reviewing it.

@natbraun
Copy link
Author

@fmoralesc, thanks! Really appreciate it. I'll keep rebasing this branch as the master branch advances, so it's up to date. Now, I'd like to contribute to neovim in general. How can I help? Thanks!

@fmoralesc
Copy link
Contributor

How can I help?

@natbraun Just as you did!

I don't know what's the general status of things, but maybe check on some of the issues for 0.2 and see if something grabs your interest -- or if you have an idea, put it out, research how to implement it and try to do it. There's also the issue of backporting patches from vim, which while tedious at times, gives you some familiarity with the codebase. Any task will give you an appreciation of how the project works, and the "intangibles", like "who's who" and such. People will help, most are friendly -- and if not, at least they are on point ;)

@fmoralesc fmoralesc changed the title Add optional third tab character in "listchars" [RFC] Add optional third tab character in "listchars" Nov 5, 2015
@marvim marvim added the RFC label Nov 5, 2015
src/nvim/option.c:3358:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
src/nvim/screen.c:2763:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/nvim/screen.c:2763:  Use false instead of FALSE.  [readability/bool] [4]
Braces in various places
* If two characters are specifed, do as before
* If three characters are specified, e.g. <->:
   - if the tab length is 1: display the third character: >
   - if the tab length is 2: display the first and third characters: <>
   - otherwise display the first, repeat the second, display the third: <--->
@cjoach
Copy link
Contributor

cjoach commented Jan 16, 2016

Do you think you could also use the test file to test if all works when only two characters? Maybe it doesn't change anything but curious.

@k-takata
Copy link
Contributor

Just FYI, this was merged into Vim 8.1.0759 (vim/vim#3810).

@justinmk justinmk added the has:vim-patch issue is fixed in vim and patch needs to be ported label Jan 25, 2019
@justinmk justinmk modified the milestones: 0.6, 0.4 Jan 25, 2019
@justinmk
Copy link
Member

@k-takata Thanks for the notice!

Merged in #9539

@justinmk justinmk closed this Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has:vim-patch issue is fixed in vim and patch needs to be ported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants