Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Wildcard analyzer helpers #578

Merged
merged 2 commits into from
Nov 30, 2023
Merged

Wildcard analyzer helpers #578

merged 2 commits into from
Nov 30, 2023

Conversation

MBkkt
Copy link
Contributor

@MBkkt MBkkt commented Nov 28, 2023

No description provided.

@MBkkt MBkkt force-pushed the wildcard-analyzer branch from 338f5c7 to df94567 Compare November 30, 2023 03:05
Copy link
Contributor

@Dronplane Dronplane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. The only case - hiding json parsing error. Old code reported such cases, new one hides actual error description.


return false;
std::shared_ptr<velocypack::Builder> ParseArgs(std::string_view args) try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noexcept?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and BTW why hide possible velocypack exception?

input = input.get(kNumHashes);
if (!input.isNumber<uint32_t>()) {
IRS_LOG_ERROR(
absl::StrCat(kNumHashes, " attribute must be uint32_t", kParseError));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
absl::StrCat(kNumHashes, " attribute must be uint32_t", kParseError));
absl::StrCat(kNumHashes, " attribute must be a positive number", kParseError));

@MBkkt MBkkt requested a review from Dronplane November 30, 2023 12:54
Copy link
Contributor

@Dronplane Dronplane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MBkkt MBkkt merged commit 1605954 into master Nov 30, 2023
1 check passed
@MBkkt MBkkt deleted the wildcard-analyzer branch November 30, 2023 13:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants