Fix minor hiliting issues in clickhouse-format #47610

murfel · 2023-03-15T11:55:39Z

Description

Fixes #45668. Added tests to confirm issues existed and are fixed correctly.

Changelog category (leave one):

Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Minor hiliting issues fixed in clickhouse-format.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

src/Parsers/ASTDictionaryAttributeDeclaration.cpp

src/Parsers/ASTWithAlias.cpp

src/Parsers/ASTWithElement.cpp

src/Parsers/ASTDictionary.cpp

src/Parsers/tests/gtest_format_hiliting.cpp

src/Parsers/ASTFunction.cpp

src/Parsers/tests/gtest_format_hiliting.cpp

Felixoid · 2023-04-03T13:05:29Z

The bugfix checks if there's a bug in the master, that would fail with new or changed tests in the PR.

@vdimir do you have, maybe, thoughts regarding improving this place?

vdimir · 2023-04-03T14:10:17Z

Bugfix validate check checks only stateless and integration tests against master.
We can ignore it if tests are hard to add, failure is unstable, or another kind of test is added (unit test in this case).

@Felixoid I think we can add some explicit message to make it clear in which cases it may fail, so it's up to the reviewer to check whether tests are added or not needed. Also, we can force green check if any unit tests are changed.

…to the pointer

… than return it, to avoid code duplication in are_equal_with_hilites

…l_with_hilites if anyone needs it, they can call it themselves

murfel · 2023-04-04T15:07:52Z

@rschu1ze thank you for the help with the build! (Among the other types of help!)

The PR is now ready to be reviewed, I don't have any plans to add anything else.

rschu1ze · 2023-04-04T15:53:24Z

Approved, thanks. Will merge once builds/tests are green.

Only pushed a smallish commit which uses a dark magic globbing macro to build the new utility library. Had to move "gest_hilite_comparator.cpp" one subdirectory deeper so it would not be accidentally be globbed as well.

murfel · 2023-04-04T19:44:49Z

Approved, thanks. Will merge once builds/tests are green.

Only pushed a smallish commit which uses a dark magic globbing macro to build the new utility library. Had to move "gest_hilite_comparator.cpp" one subdirectory deeper so it would not be accidentally be globbed as well.

Yay! That's my first non-trivially sized PR into ClickHouse.

Thank you for the review and your input!

murfel changed the title ~~add a hilite-aware equality comparator and a sample test~~ WIP: Fix minor hiliting issues in clickhouse-format Mar 15, 2023

robot-ch-test-poll2 added the pr-bugfix Pull request with bugfix, not backported by default label Mar 15, 2023

murfel marked this pull request as draft March 15, 2023 12:06

murfel marked this pull request as ready for review March 15, 2023 23:58

murfel marked this pull request as draft March 16, 2023 00:00

This comment was marked as outdated.

Sign in to view

rschu1ze self-assigned this Mar 16, 2023

qoega added the can be tested Allows running workflows for external contributors label Mar 16, 2023

rschu1ze added can be tested Allows running workflows for external contributors and removed can be tested Allows running workflows for external contributors labels Mar 16, 2023

This comment was marked as outdated.

Sign in to view

rschu1ze reviewed Mar 16, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

rschu1ze reviewed Mar 21, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

murfel mentioned this pull request Mar 24, 2023

WIP: Refactor IAST::FormatSettings #45649

Draft

1 task

rschu1ze reviewed Mar 28, 2023

View reviewed changes

src/Parsers/ASTFunction.cpp Outdated Show resolved Hide resolved

rschu1ze approved these changes Mar 28, 2023

View reviewed changes

src/Parsers/tests/gtest_format_hiliting.cpp Outdated Show resolved Hide resolved

robot-ch-test-poll4 added the submodule changed At least one submodule changed in this PR. label Mar 28, 2023

This comment was marked as outdated.

Sign in to view

murfel added 5 commits March 28, 2023 17:21

add a hilite-aware equality comparator and a sample test

103bb9b

cleanup

086fe07

add failing tests

da52850

fix bugs (now tests pass)

ed22d0f

compare string without hilites first, for easier debug

bf4a3a2

Fix build

c8ef87f

This comment was marked as outdated.

Sign in to view

murfel added 16 commits April 4, 2023 11:27

move hilite comparator tests as well

779bd3b

fix hilite comparator tests

880e441

move const var definition in HiliteComparator

3452e19

extract HiliteComparator tests into an own test suit

3e11c06

restructure format hiliting tests to avoid code duplication

ebb8899

change compare argument order to be expected, query (=actual)

2ff5d60

HiliteComparator: add Hilite alias for const char * representing hilites

86edd48

Fix style

2e9607f

accept the pointer by & in consume_hilites, rather than by a pointer …

f3560a2

…to the pointer

refactor consume_hilites to assign last_hilite to an argument, rather…

0a021d3

… than return it, to avoid code duplication in are_equal_with_hilites

avoid explicit check if strings are_equal_without_hilites in are_equa…

f6db4fa

…l_with_hilites if anyone needs it, they can call it themselves

fix while in consume_hilites

cba124f

update docstring for consume_hilites

a1d718c

add check that all strings end without any hilite

46691ef

add comment re excessive copying in are_equal_with_hilites_removed

95d7719

Merge remote-tracking branch 'origin/master' into format-hiliting-fix

a1d3ea9

murfel marked this pull request as ready for review April 4, 2023 14:59

murfel added 2 commits April 4, 2023 16:01

rm unneeded parentheses

647fb63

fix style - missed tab

c3acaec

rschu1ze changed the title ~~WIP: Fix minor hiliting issues in clickhouse-format~~ Fix minor hiliting issues in clickhouse-format Apr 4, 2023

More CMake fixes

e031783

rschu1ze approved these changes Apr 4, 2023

View reviewed changes

rschu1ze merged commit db86489 into ClickHouse:master Apr 4, 2023
134 of 138 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix minor hiliting issues in clickhouse-format #47610

Fix minor hiliting issues in clickhouse-format #47610

murfel commented Mar 15, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Felixoid commented Apr 3, 2023

This comment was marked as outdated.

vdimir commented Apr 3, 2023

murfel commented Apr 4, 2023

rschu1ze commented Apr 4, 2023

murfel commented Apr 4, 2023 •

edited

Fix minor hiliting issues in clickhouse-format #47610

Fix minor hiliting issues in clickhouse-format #47610

Conversation

murfel commented Mar 15, 2023

Description

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Felixoid commented Apr 3, 2023

This comment was marked as outdated.

vdimir commented Apr 3, 2023

murfel commented Apr 4, 2023

rschu1ze commented Apr 4, 2023

murfel commented Apr 4, 2023 • edited

murfel commented Apr 4, 2023 •

edited