Skip to content

Commit

Permalink
Add initial support for an escape character (#2361)
Browse files Browse the repository at this point in the history
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 #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 #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 #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
  • Loading branch information
Filip62 committed Jan 26, 2021
1 parent eaa5069 commit 4ded401
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 1 deletion.
10 changes: 10 additions & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]
### Breaking
- We added the backslash escape character (\\) for configuration values. This
means that the literal backslash character now has special meaning in
configuration files, therefore if you want to use it in a value as a literal
backslash, you need to escape it with the backslash escape character. The
parser logs an error if any unescaped backslashes are found in a value. This
affects you only if you are using two consecutive backslashes in a value,
which will now be interpreted as a single literal backslash.
[`#2354`](https://github.com/polybar/polybar/issues/2354)
- We rewrote our tag parser. This shouldn't break anything, if you experience
any problems, please let us know.
The new parser now gives errors for certain invalid tags where the old parser
Expand Down Expand Up @@ -42,6 +50,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
repository.

### Added
- The backslash escape character (\\).
[`#2354`](https://github.com/polybar/polybar/issues/2354)
- Warn states for the cpu, memory, fs, and battery modules.
([`#570`](https://github.com/polybar/polybar/issues/570),
[`#956`](https://github.com/polybar/polybar/issues/956),
Expand Down
17 changes: 16 additions & 1 deletion doc/man/polybar.5.rst
Expand Up @@ -128,7 +128,22 @@ in double-quotes:

name = " value "

Here ``name`` has a leading and trailing whitespace.
Here the value of the ``name`` key has a leading and trailing whitespace.

To treat characters with special meaning as literal characters, you need to
prepend them with the backslash (``\``) escape character:

::

name = "value\\value\\value"

Value of this key ``name`` results in ``value\value\value``.

.. note::

The only character with a special meaning right now is the backslash character
(``\``), which serves as the escape character.
More will be added in the future.

Empty Lines & Comments
^^^^^^^^^^^^^^^^^^^^^^
Expand Down
6 changes: 6 additions & 0 deletions include/components/config_parser.hpp
Expand Up @@ -190,6 +190,12 @@ class config_parser {
*/
std::pair<string, string> parse_key(const string& line);

/**
* \brief Parses the given value, checks if the given value contains
* one or more unescaped backslashes and logs an error if yes
*/
string parse_escaped_value(string&& value, const string& key);

/**
* \brief Name of all the files the config includes values from
*
Expand Down
23 changes: 23 additions & 0 deletions src/components/config_parser.cpp
Expand Up @@ -257,6 +257,8 @@ std::pair<string, string> config_parser::parse_key(const string& line) {
throw invalid_name_error("Key", key);
}

value = parse_escaped_value(move(value), key);

/*
* Only if the string is surrounded with double quotes, do we treat them
* not as part of the value and remove them.
Expand Down Expand Up @@ -292,4 +294,25 @@ bool config_parser::is_valid_name(const string& name) {
return true;
}

string config_parser::parse_escaped_value(string&& value, const string& key) {
string cfg_value = value;
bool log = false;
auto backslash_pos = value.find('\\');
while (backslash_pos != string::npos) {
if (backslash_pos == value.size() - 1 || value[backslash_pos + 1] != '\\') {
log = true;
} else {
value = value.replace(backslash_pos, 2, "\\");
}
backslash_pos = value.find('\\', backslash_pos + 1);
}
if (log) {
// TODO Log filename, and line number
m_log.err(
"Value '%s' of key '%s' contains one or more unescaped backslashes, please prepend them with the backslash "
"escape character.",
cfg_value, key);
}
return move(value);
}
POLYBAR_NS_END
40 changes: 40 additions & 0 deletions tests/unit_tests/components/config_parser.cpp
Expand Up @@ -24,6 +24,9 @@ class TestableConfigParser : public config_parser {
public:
using config_parser::parse_line;

public:
using config_parser::parse_escaped_value;

public:
using config_parser::m_files;
};
Expand Down Expand Up @@ -64,6 +67,12 @@ vector<pair<pair<string, string>, string>> parse_line_key_list = {
{{"key", "value"}, " key = value"},
{{"key", ""}, " key\t = \"\""},
{{"key", "\""}, " key\t = \"\"\""},
{{"key", "\\"}, " key = \\"},
{{"key", "\\"}, " key = \\\\"},
{{"key", "\\val\\ue\\"}, " key = \\val\\ue\\"},
{{"key", "\\val\\ue\\"}, " key = \\\\val\\\\ue\\\\"},
{{"key", "\\val\\ue\\"}, " key = \"\\val\\ue\\\""},
{{"key", "\\val\\ue\\"}, " key = \"\\\\val\\\\ue\\\\\""},
};

INSTANTIATE_TEST_SUITE_P(Inst, ParseLineInValidTest, ::testing::ValuesIn(parse_line_invalid_list));
Expand Down Expand Up @@ -236,3 +245,34 @@ TEST_F(ParseHeaderTest, throwsSyntaxError) {
EXPECT_THROW(parser->parse_header("[root]"), syntax_error);
}
// }}}

// ParseEscapedValueTest {{{

/**
* \brief Class for parameterized tests on parse_escaped_value
*
* The first element of the pair is the expected value and the second
* element is the escaped string to be parsed.
*/
class ParseEscapedValueTest : public ConfigParser, public ::testing::WithParamInterface<pair<string, string>> {};

vector<pair<string, string>> parse_escaped_value_list = {
{"\\", "\\"},
{"\\", "\\\\"},
{"\\val\\ue\\", "\\val\\ue\\"},
{"\\val\\ue\\", "\\\\val\\\\ue\\\\"},
{"\"\\val\\ue\\\"", "\"\\val\\ue\\\""},
{"\"\\val\\ue\\\"", "\"\\\\val\\\\ue\\\\\""},
};

INSTANTIATE_TEST_SUITE_P(Inst, ParseEscapedValueTest, ::testing::ValuesIn(parse_escaped_value_list));

/**
* Parameterized test for parse_escaped_value with valid line
*/
TEST_P(ParseEscapedValueTest, correctness) {
string value = GetParam().second;
value = parser->parse_escaped_value(move(value), "key");
EXPECT_EQ(GetParam().first, value);
}
// }}}

0 comments on commit 4ded401

Please sign in to comment.