Skip to content

Conversation

@felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Dec 20, 2023

Rationale for this change

This simplifies the creation of long error messages and leads to the use of a string builder to construct the error message.

What changes are included in this PR?

  • std::forward in ExceptionToStatus
  • A few nitpicky changes
  • Simplification of the error message text
  • Moving the signature of CheckIfHierarchicalNamespaceIsEnabled to azurefs_internal.h to reduce the size of azurefs.h -- implementation remains in azurefs.cc

Are these changes tested?

Yes. By existing tests.

This simplifies the creation of long error messages and leads to the use
of a string builder to construct the error message.
…ernal.h

But keep implementation in azurefs.cc to avoid having to expose
currently azurefs.cc-private symbols in headers.
@felipecrv felipecrv requested a review from pitrou December 20, 2023 15:28
@felipecrv
Copy link
Contributor Author

@Tom-Newton @kou

@github-actions
Copy link

⚠️ GitHub issue #39322 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Neat improvement.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 20, 2023
@felipecrv felipecrv requested a review from pitrou December 20, 2023 16:48
@pitrou
Copy link
Member

pitrou commented Dec 20, 2023

It seems you need to fix or circumvent the linting failure.

@felipecrv
Copy link
Contributor Author

It seems you need to fix or circumvent the linting failure.

Trying something! 🤞

@felipecrv felipecrv merged commit 91b2243 into apache:main Dec 20, 2023
@felipecrv felipecrv removed the awaiting committer review Awaiting committer review label Dec 20, 2023
@felipecrv felipecrv deleted the forward_args branch December 20, 2023 18:29
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 91b2243.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…way to Status::FromArgs (apache#39323)

### Rationale for this change

This simplifies the creation of long error messages and leads to the use of a string builder to construct the error message.

### What changes are included in this PR?

 - std::forward in ExceptionToStatus
 - A few nitpicky changes
 - Simplification of the error message text
 - Moving the signature of `CheckIfHierarchicalNamespaceIsEnabled` to `azurefs_internal.h` to reduce the size of `azurefs.h` -- implementation remains in `azurefs.cc`

### Are these changes tested?

Yes. By existing tests.
* Closes: apache#39322

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][FS] Forward arguments to ExceptionToStatus

2 participants