Fokko merged PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apach
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612114917
> TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring
to type it for every function and make the function signature too verbose. How
about only adding `[[nodiscard]]`
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612095523
Hi @Xuanwo @Fokko , I think this PR is ready to merge, please take a look
when you got the time.
--
This is an automated message from the Apache Git Service.
To respond to the message,
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612048913
> > I checked that while working on this PR, both '' and file works.
>
> Thanks for confirming, I didn't see the `file` documented and was
wondering why we had it.
Let's chan
raulcd commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612046285
> I checked that while working on this PR, both '' and file works.
Thanks for confirming, I didn't see the `file` documented and was wondering
why we had it.
--
This is an automat
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612040409
> I have never configured the cpp-linter action previously but don't we have
to update the [tidy-checks
option](https://cpp-linter.github.io/cpp-linter-action/inputs-outputs/#tidy-checks)
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611997925
nit: change the PR title to `ci: add clang-tidy check`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL abov
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611877332
Ah, interesting. (I am used to the std::expected backport which doesn't do
this, so I always put it by hand everywhere...)
Anyways, I don't feel strongly about this. If you want to
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611769691
> If we are going to use a Result/Status object, then I think we should have
`[[nodiscard]]` enforcement. If we are going to use exceptions then I don't
feel so strongly.
Then we ca
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611736128
If we are going to use a Result/Status object, then I think we should have
`[[nodiscard]]` enforcement. If we are going to use exceptions then I don't
feel so strongly.
--
This is an
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611699882
> TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring
to type it for every function and make the function signature too verbose. How
about only adding `[[nodiscard]]
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611657586
TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring to
type it for every function and make the function signature too verbose. How
about only adding `[[nodiscard]]` t
zhjwpku commented on code in PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#discussion_r1927244347
##
.clang-tidy:
##
@@ -0,0 +1,37 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# dis
lidavidm commented on code in PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#discussion_r1927188402
##
.clang-tidy:
##
@@ -0,0 +1,37 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# di
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2610134996
Sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2610043044
As @lidavidm is working on the type system, we seems to prefer Arrow-style,
so I'm proposing we adopt Arrow's clang-tidy.
There are two points I want to mention here:
- I did not
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2602114984
I found a clang-tidy readability-identifier-naming for Google's naming
convention [1], the gist has 7 revisions and seems quite popular(18 stars),
does it make sense we adopt it?
[
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601952277
Most of it is enforced by committers on PRs. It would probably be nice to
have more automated enforcement, though.
--
This is an automated message from the Apache Git Service.
To respond
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601912091
> I don't think we've had any major issues with our style conventions on
Arrow C++.
>
> The only annoyance I have personally is the ambiguity around method
naming, between `CamelCa
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601757647
I don't think we've had any major issues with our style conventions on Arrow
C++.
The only annoyance I have personally is the ambiguity around method naming,
between `CamelCase` for
gaborkaszab commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601730266
> Should we just follow what
[Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)
does and adjust it as needed? I think it is based on the
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597414406
> As the docs state, that's for Objective-C, so I wouldn't expect it to help
Yeah, I did not notice that ;(
--
This is an automated message from the Apache Git Service.
To respond
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597391057
As the docs state, that's for Objective-C, so I wouldn't expect it to help
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHu
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597357820
> Should we just follow what
[Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)
does and adjust it as needed? I think it is based on the Goog
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597296054
> Should we just follow what
[Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)
does and adjust it as needed? I think it is based on the Goo
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597283325
> More generally: is there a particular style guide (e.g. Google, LLVM, ...)
that we want to adopt?
I didn't find such one line setting, this PR sets lots of
readability-identifier
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597282759
Should we just follow what
[Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci)
does and adjust it as needed? I think it is based on the Google
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597182577
More generally: is there a particular style guide (e.g. Google, LLVM, ...)
that we want to adopt?
--
This is an automated message from the Apache Git Service.
To respond to the message
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2596010890
This is the outcome of @lidavidm 's question whether we should use
lowerCamelCase or UpperCamelCase for method names. I referenced ORC and Arrow's
clang-tidy configs.
If a PR want
29 matches
Mail list logo
