You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/02/20 22:06:42 UTC
[GitHub] [incubator-iceberg] vrozov opened a new pull request #816: Remove
all exceptions from AvoidStaticImport checkstyle rule
vrozov opened a new pull request #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] vrozov closed pull request #816: Remove all
exceptions from AvoidStaticImport checkstyle rule
Posted by GitBox <gi...@apache.org>.
vrozov closed pull request #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #816: Remove all
exceptions from AvoidStaticImport checkstyle rule
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816#issuecomment-589391601
+1 for the approach Anton suggests. That will apply the rule more clearly, but won't require near as many changes that are going to cause conflicts. Since this is a new project, many people are maintaining branches so we need to be careful about the amount of churn this kind of thing introduces.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] vrozov commented on issue #816: Remove all
exceptions from AvoidStaticImport checkstyle rule
Posted by GitBox <gi...@apache.org>.
vrozov commented on issue #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816#issuecomment-589445021
I don't have a strong opinion on what static imports should be allowed and which one should be avoided (my personal preference is to allow them all). Assuming that the community prefers to disallow static imports, IMO, it will be better to limit the number of exceptions. No exceptions is the best scenario, and cherry-pick of few packages/classes is the worst scenario with the org.apache.iceberg.* exception being in the middle.
@rdblue Note that almost half of files modified belong to spark jmh that most likely are not changed on a large number of branches and/or forks.
At this point, it will be better to go back to the dev list and discuss how static imports should be handled there as this PR implements what was discussed and agreed on the dev list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on issue #816: Remove all
exceptions from AvoidStaticImport checkstyle rule
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on issue #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816#issuecomment-589388986
There was a discussion on the dev list but it seems like a lot of changes to me.
Personally, it made sense to allow static imports for Iceberg-specific classes such as table properties, types, expressions. The code looked concise and straightforward. I do get the reason to limit the number of static imports but I would focus only on external things like below:
```
java.util.Collections.*,
java.util.stream.Collectors.*,
com.palantir.logsafe.Preconditions.*,
com.google.common.base.Preconditions.*,
org.apache.commons.lang3.Validate.*,
org.junit.Assert.*,
```
In the community, we are trying to avoid those anyway but our checks don't enforce that. Hence, new contributors frequently use static imports for those classes and we ask them to correct such places.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on issue #816: Remove all
exceptions from AvoidStaticImport checkstyle rule
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on issue #816: Remove all exceptions from AvoidStaticImport checkstyle rule
URL: https://github.com/apache/incubator-iceberg/pull/816#issuecomment-589389525
Having said that, it is just an opinion. I will be okay if we decide to be more restrictive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org