Add initial support for an escape character #2354 #2361

Filip62 · 2021-01-21T14:52:35Z

What type of PR is this? (check all applicable)

Description

Introduce an escape character '\'.
Warn users about unescaped characters (only backslashes for now).
For now, treat a single unescaped backslash as a literal backslash character.
Treat two consecutive backslashes as a single literal backslash character.

Related Issues & Documents

Implements first step of #2354

Documentation (check all applicable)

This PR requires changes to the Wiki documentation (describe the changes)
See Escape characters in configs #2354
This PR requires changes to the documentation inside the git repo (please add them to the PR).
Does not require documentation changes

codecov · 2021-01-21T14:55:45Z

Codecov Report

Merging #2361 (00b60ae) into master (eaa5069) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           master   #2361      +/-   ##
=========================================
+ Coverage    7.96%   8.10%   +0.13%     
=========================================
  Files         143     143              
  Lines       10369   10382      +13     
=========================================
+ Hits          826     841      +15     
+ Misses       9543    9541       -2

Flag	Coverage Δ
unittests	`8.10% <100.00%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
include/components/config_parser.hpp	`66.66% <ø> (ø)`
src/components/config_parser.cpp	`49.32% <100.00%> (+4.87%)`	⬆️
include/components/logger.hpp	`28.57% <0.00%> (+14.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa5069...00b60ae. Read the comment docs.

patrick96

Thanks :)

I think everything works, good job!

As far as I can tell you are using positive_offset and negative_offset to keep track of the difference in indices between cfg_value and the returned value (and escaped_value).

Now, I don't think it's necessary to generate the escaped string for when there are unescaped backslashes. In that case, it would be possible to use the same string object for searching and replacing without having to keep track of index differences.

There are also a few other things that should be in this PR:

The manpage for the polybar config should be updated to include information about the escape character. The file is in doc/man/polybar.5.rst and the changes should go in the Keys section.
New tests for the new function as well as some added testcases for the ParseLineKeyTest that include backslashes. The tests belong in tests/unit_tests/components/config_parser.cpp
A changelog entry describing your changes, including breaking changes: https://github.com/polybar/polybar/blob/master/CONTRIBUTING.md#changelog

src/components/config_parser.cpp

patrick96 · 2021-01-23T21:11:05Z

Let me know once this is ready for review again.

Also, the CI docs fail isn't your fault, there seems to be an issue with an ubuntu mirror.

patrick96

Code looks good now. Just some minor adjustments.

Also, I'm curious why you are using find_first_of instead of find

CHANGELOG.md

doc/man/polybar.5.rst

src/components/config_parser.cpp

tests/unit_tests/components/config_parser.cpp

patrick96 · 2021-01-24T14:51:10Z

Also, it seems that CI is also having a segfault. I was also able to reproduce it sometimes. Looking at the coredump, it looks like it's something inside the logger, but the actual errors are kind of weird. I suspect that it's because you used string concatenation for the log message.

Filip62 · 2021-01-24T15:26:37Z

Code looks good now. Just some minor adjustments.

Also, I'm curious why you are using find_first_of instead of find

No reason, changed it to find.

Also, it seems that CI is also having a segfault. I was also able to reproduce it sometimes. Looking at the coredump, it looks like it's something inside the logger, but the actual errors are kind of weird. I suspect that it's because you used string concatenation for the log message.

Printf tokens didn't help.

patrick96

I think I found the issue with the segfault. It was undefined behavior in test code I wrote.

I pushed a fix in #2364.

After that is merged we can see if there are any other CI issues with your code.
But other than that, I think we can merge this soon.

src/components/config_parser.cpp

patrick96 · 2021-01-26T15:29:51Z

Can you rebase against master so that your branch includes my fix?

Filip62 · 2021-01-26T16:24:43Z

If we remove the backslash_pos == value.size() - 1 check, why would it lead to out of bounds access?
Wouldn't it be just looking at the string termination character?

patrick96 · 2021-01-26T16:32:27Z

Everything looks good now, but I can't merge yet because there is still a merge conflict (though it wasn't there a minute ago). You need to resolve it before I can merge this.

Filip62 · 2021-01-26T16:35:56Z

Everything looks good now, but I can't merge yet because there is still a merge conflict (though it wasn't there a minute ago). You need to resolve it before I can merge this.

I am not sure if I am missing something but git merge upstream/master tells me I am up to date.

Upstream being your polybar repo.

patrick96 · 2021-01-26T16:44:58Z

I think you need to to git rebase upstream/master.

patrick96 · 2021-01-26T16:45:55Z

I think 6b21044 messed things up a bit

Filip62 · 2021-01-26T17:05:49Z

Sorry for breaking things, still get confused by git sometimes.

Add a config parser method which, for now, deals only with escaping the literal backslash character and logs an error message to inform the user of the coming change. The error message includes a properly escaped value for the user. As a result of introducing an escape character('\'): - Warn the user of any unescaped backslashes, as they will not be treated as a literal character in the future - For now, still treat a single backslash as a literal character - Treat two consecutive backslashes as a single properly escaped literal backslash Also: - Add documentation about the escape character to polybar(5) manpage - Add info about the escape character to changelog - Add testcases for ParseLineKeyTest - Add new test ParseEscapedValueTest Resolves: First step in polybar#2354 Improve value parsing - Take value arg in as an rvalue reference and move parsed value back - Remove unnecessary if statement - Rename function - Improve error message - Improve function description - Format Add escape character documentation to manpages Add information about the escape character to the polybar(5) manpage. Add info about the esacape character to changelog Add test cases for ParseLineKeyTest Fix ParseLineKeyTest test cases Also make config parser method parse_escaped_value private. Add tests for escaped_value_parser method Also remove unsued include statement Simplify parse_escaped_value in config_parser Remove unnecessary escaped value generation, so we do not have to keep track of index differences. Fix ParseEscapedValueTest test cases Fix parse_escaped_value Add more test cases for ParseLineKeyTest Update CHANGELOG.md Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com> Adress review - Adjust documentation - Small code changes Improve parse_escaped_value Add initial support for an escape character Add a config parser method which, for now, deals only with escaping the literal backslash character and logs an error message to inform the user of the coming change. The error message includes a properly escaped value for the user. As a result of introducing an escape character('\'): - Warn the user of any unescaped backslashes, as they will not be treated as a literal character in the future - For now, still treat a single backslash as a literal character - Treat two consecutive backslashes as a single properly escaped literal backslash Resolves: First step in polybar#2354 Improve value parsing - Take value arg in as an rvalue reference and move parsed value back - Remove unnecessary if statement - Rename function - Improve error message - Improve function description - Format Add info about the esacape character to changelog Add test cases for ParseLineKeyTest Fix ParseLineKeyTest test cases Also make config parser method parse_escaped_value private. Add tests for escaped_value_parser method Also remove unsued include statement Simplify parse_escaped_value in config_parser Remove unnecessary escaped value generation, so we do not have to keep track of index differences. Fix ParseEscapedValueTest test cases Add more test cases for ParseLineKeyTest Adress review - Adjust documentation - Small code changes Remove duplicate testcase from ParseLineKeyTest Add initial support for an escape character Add a config parser method which, for now, deals only with escaping the literal backslash character and logs an error message to inform the user of the coming change. The error message includes a properly escaped value for the user. As a result of introducing an escape character('\'): - Warn the user of any unescaped backslashes, as they will not be treated as a literal character in the future - For now, still treat a single backslash as a literal character - Treat two consecutive backslashes as a single properly escaped literal backslash Resolves: First step in polybar#2354 Improve value parsing - Take value arg in as an rvalue reference and move parsed value back - Remove unnecessary if statement - Rename function - Improve error message - Improve function description - Format Fix ParseLineKeyTest test cases Also make config parser method parse_escaped_value private. Remove duplicate testcase from ParseLineKeyTest

patrick96 · 2021-01-26T18:06:13Z

There was still an issue. I went ahead and just squashed all commits.

patrick96 · 2021-01-26T18:16:46Z

Done! Thanks a lot for all your efforts 😃

Filip62 · 2021-01-26T18:18:15Z

Thanks a lot for all YOUR efforts.

Also, if you don't mind:

If we remove the backslash_pos == value.size() - 1 check, why would it lead to out of bounds access?
Wouldn't it be just looking at the string termination character?

patrick96 · 2021-01-26T20:09:06Z

C++ strings don't have a termination character. If you do str[str.size()], you will access one element to the right of the last one.
The std::string class stores the string size separately and doesn't need a termination character.

Filip62 · 2021-01-26T20:18:48Z

I should've known that, I know the size is stored internally, but no one ever told me that the null termination is not there in C++, I am amazed and embarrassed.

patrick96 requested changes Jan 22, 2021

View reviewed changes

src/components/config_parser.cpp Outdated Show resolved Hide resolved

src/components/config_parser.cpp Outdated Show resolved Hide resolved

src/components/config_parser.cpp Outdated Show resolved Hide resolved

src/components/config_parser.cpp Outdated Show resolved Hide resolved

patrick96 requested changes Jan 24, 2021

View reviewed changes

patrick96 requested changes Jan 26, 2021

View reviewed changes

src/components/config_parser.cpp Outdated Show resolved Hide resolved

Filip62 marked this pull request as ready for review January 26, 2021 16:18

Filip62 requested a review from patrick96 January 26, 2021 16:22

patrick96 approved these changes Jan 26, 2021

View reviewed changes

patrick96 force-pushed the feature/escape-characters branch from 0305c15 to 00b60ae Compare January 26, 2021 18:05

patrick96 merged commit 4ded401 into polybar:master Jan 26, 2021

Uh oh!

Add initial support for an escape character #2354 #2361

Add initial support for an escape character #2354 #2361

Uh oh!

Conversation

Filip62 commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

Documentation (check all applicable)

Uh oh!

codecov bot commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick96 commented Jan 23, 2021

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick96 commented Jan 24, 2021

Uh oh!

Filip62 commented Jan 24, 2021

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

Filip62 commented Jan 26, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

Filip62 commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

Filip62 commented Jan 26, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

Filip62 commented Jan 26, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

Filip62 commented Jan 26, 2021

Uh oh!

Uh oh!

Filip62 commented Jan 21, 2021 •

edited

Loading

codecov bot commented Jan 21, 2021 •

edited

Loading

Filip62 commented Jan 26, 2021 •

edited

Loading