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

Trim trailing whitespace in VundleLog #671

Closed
wants to merge 1 commit into from
Closed

Trim trailing whitespace in VundleLog #671

wants to merge 1 commit into from

Conversation

nfischer
Copy link
Contributor

Trims trailing white space in the Vundle log file with :s/\s\+//e and then restores cursor position and the search register's previous value. This should be relatively side-effect free.

@gmarik
Copy link
Contributor

gmarik commented Nov 18, 2015

@nfischer would it be better if trailing spaces get trimmed when log is generated?
No need to deal with cursor and all that…

@nfischer
Copy link
Contributor Author

Good catch. I agree, that's the better way to do it. I updated this with the new fix. Commits have been squashed.

@nfischer
Copy link
Contributor Author

Just updated to move some work outside of the for loop, so performance may be slightly improved. Should be ready for review now.

for line in lines
call add(g:vundle#log, '['. time .'] '. prefix . line)
" Trim trailing whitespace
let entry = substitute(entry_prefix . line, '\m\s\+$', '', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be just entry_prefix.substitute(line, '\m\s\+$', '', '') instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This falls apart for the case where line is the empty string. This would append the empty string onto the end, but wouldn't fix the trailing whitespace in entry_prefix (since we put a space between the time stamp and the prefix. So I believe this approach is a more robust way to go

@gmarik
Copy link
Contributor

gmarik commented Nov 20, 2015

also could you please attach a screenshot of the issue? Just for the record…

@nfischer
Copy link
Contributor Author

vundlelog

The trailing whitespace is marked by the teal dots at the end of lines (each dot represents a trailing whitespace character). A lot of this is introduced as an effect of how the log entries are formed, although this could also be caused by trailing whitespace in the line parameter as well.

@nfischer
Copy link
Contributor Author

nfischer commented Mar 8, 2016

@ryanoasis @gmarik pinging this thread, to see if this can be merged or should be closed

@ryanoasis
Copy link
Member

@nfischer I think this should be merged. When I get time I plan on testing it (just hasn't happened yet) 😞

@nfischer
Copy link
Contributor Author

nfischer commented Mar 8, 2016

@ryanoasis Ok awesome! It's intended to be a fairly harmless change, it just makes things look nicer for people like me who highlight trailing white space. And it makes things nicer if #683 is merged.

@ryanoasis ryanoasis modified the milestone: 0.10.3 Mar 16, 2016
ryanoasis added a commit that referenced this pull request Apr 22, 2016
Trim trailing whitespace in VundleLog
@ryanoasis
Copy link
Member

Finally got around to getting these into branch/release 0.10.3

Have only cursory tested :VundleInstall, :VundleUpdate, :VundleSearch, and viewed the vundle log.

Side note, not sure when it happened (seems to even happen on master) but during my testing :VundleUpdate is VERY SLOW... maybe it is just on my end?

@nfischer
Copy link
Contributor Author

Just tried this out on the new branch, and :VundleUpdate seems to be ok to me

@ryanoasis
Copy link
Member

Thanks for checking. I'm going to retest today I think something funky may have been happening with my connection

@nfischer nfischer closed this Jul 2, 2018
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.

3 participants