Skip to content

don't change colours on blank lines#31

Closed
landfillbaby wants to merge 1 commit intolunasorcery:mainfrom
landfillbaby:blankline
Closed

don't change colours on blank lines#31
landfillbaby wants to merge 1 commit intolunasorcery:mainfrom
landfillbaby:blankline

Conversation

@landfillbaby
Copy link
Contributor

builds on @obfusk's changes. fixes #24

@obfusk
Copy link
Contributor

obfusk commented Jun 21, 2020

The variable names are a bit inconsistent now: g_blankLine vs g_changeEmpty.
I originally only skipped "empty" lines. But now the code also handles "blank" (i.e. empty or only whitespace) lines.
I suggest renaming variables and command line options to use "blank".

@landfillbaby landfillbaby force-pushed the blankline branch 2 times, most recently from 187e8ff to 59ff502 Compare June 21, 2020 22:15
@landfillbaby
Copy link
Contributor Author

how's this?

@obfusk
Copy link
Contributor

obfusk commented Jun 21, 2020

LGTM. I think it would be better to switch Author/Co-authored-by though.

You could maybe add \f and \v as blank characters.
Full-width spaces ( ) might be a bit difficult to detect with the current code.
Not sure if there are other blank characters...

(<shameless plug>my python version skips all of these 😅</shameless plug>)

@landfillbaby
Copy link
Contributor Author

done

@obfusk
Copy link
Contributor

obfusk commented Jun 21, 2020

Should

		if (c == '\n' || c == '\f' || c == '\v') {
[...]
		(c != ' ' && c != '\t' && c != '\r'))) {

not be

		if (c == '\n') {
[...]
		(c != ' ' && c != '\t' && c != '\r' && c != '\f' && c != '\v'))) {

instead?

@obfusk
Copy link
Contributor

obfusk commented Jun 22, 2020

How about this?

#include <cctype>
[...]
else if (g_blankLine && (g_changeBlank || !isspace(c))) {

@landfillbaby
Copy link
Contributor Author

i was thinking that since \f and \v both do effectively cause a new line, (at least they do on Android Termux, my PC is out of commission,) they should be treated as such, but if you think they should be ignored like the horizontal whitespace we can change it
and yes maybe isspace would be good to use

@obfusk
Copy link
Contributor

obfusk commented Jun 22, 2020

I think \f and \v should normally not be found in the input anyway (I don't recall ever using them), and -- since different terminals don't always handle them identically -- only treating \n as newline is best. And isspace also makes the code a bit cleaner :) But I think there is probably no "correct answer" for all situations.

@obfusk
Copy link
Contributor

obfusk commented Jun 22, 2020

Throw in some \bs and all bets are off :)

optionally do, with -c --change-blank
consider lines with only spaces and tabs empty

Co-authored-by: Felix C. Stegerman <[email protected]>
@landfillbaby
Copy link
Contributor Author

ok yes. \n only, and !isspace(c) done

@landfillbaby
Copy link
Contributor Author

closing in favour of #33

@landfillbaby landfillbaby deleted the blankline branch June 23, 2020 19:40
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.

empty/blank lines result in "missing" colors

2 participants