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

Fix warning printed when chunk key placeholder not replaced #2523

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

dobesv
Copy link
Contributor

@dobesv dobesv commented Jul 24, 2019

This used $1 to print the key name. However, there are no capture groups in CHUNK_KEY_PLACEHOLDER_PATTERN, so $1 would not be set. According to the Ruby docs I looked up, $& can be used to get the part of the string that matched, which I think will give the correct output in this case.

This used `$1` to print the key name.  However, there are no capture groups in `CHUNK_KEY_PLACEHOLDER_PATTERN`, so `$1` would not be set.  According to the Ruby docs I looked up, `$&` can be used to get the part of the string that matched, which I think will give the correct output in this case.
@ganmacs
Copy link
Member

ganmacs commented Jul 25, 2019

there are no capture groups in CHUNK_KEY_PLACEHOLDER_PATTERN, so $1 would not be set.

Right. but I think changing the regex pattern of CHUNK_KEY_PLACEHOLDER_PATTERN from /\$\{[-_.@$a-zA-Z0-9]+\}/ to /\$\{([-_.@$a-zA-Z0-9]+)\}/ is better since we need to capture only the value of tag, not a whole of a tag.

an example using $&

CHUNK_KEY_PLACEHOLDER_PATTERN = /\$\{[-_.@$a-zA-Z0-9]+\}/

rvalue = "${a}"

if rvalue =~ CHUNK_KEY_PLACEHOLDER_PATTERN
  p $&                          # "${a}"
end

an example using /\$\{([-_.@$a-zA-Z0-9]+)\}/ for regex pattern

CHUNK_KEY_PLACEHOLDER_PATTERN = /\$\{([-_.@$a-zA-Z0-9]+)\}/

rvalue = "${a}"

if rvalue =~ CHUNK_KEY_PLACEHOLDER_PATTERN
  p $1                          #=> "a"
end

@dobesv
Copy link
Contributor Author

dobesv commented Jul 25, 2019

Personally I would prefer to see the whole ${a} but it's totally unimportant, TBH. Either would be a tremendous improvement.

@dobesv
Copy link
Contributor Author

dobesv commented Jul 25, 2019

You are welcome to make the change your way and make another PR, I won't be updating this PR.

@ganmacs
Copy link
Member

ganmacs commented Jul 26, 2019

it's not for my preference. A current implementation which does the similar thing returns the value of the tag (https://github.com/fluent/fluentd/pull/2523/files#diff-7a3f304568ba6143aaf2b7e78011d044R749). I think it's important to have consistency with other warnings.

@ganmacs ganmacs merged commit 7482802 into fluent:master Dec 12, 2019
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.

2 participants