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.

Sign up for GitHub

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

Jump to bottom

Require Assertions to be statically imported #10517

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Require Assertions to be statically imported #10517

merged 1 commit into from
Jun 18, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Jun 17, 2024

@findepi findepi mentioned this pull request Jun 17, 2024
Merged
@findepi
Copy link
Member Author

findepi commented Jun 17, 2024

currently based on #10511, #10512

@findepi findepi requested a review from nastra June 17, 2024 12:34
@findepi
Copy link
Member Author

findepi commented Jun 17, 2024

currently based on #10511, #10512

rebased, no longer the case.

@nastra
Copy link
Contributor

nastra commented Jun 17, 2024

This might take a few days to review as I just made it through the first 100 files

@findepi
Copy link
Member Author

findepi commented Jun 17, 2024

no worries, thanks for reviewing!
all the static import were added with intellij's "import statically" action
i need to rebase to resolve conflicts with #10500

@findepi
Copy link
Member Author

findepi commented Jun 17, 2024

rebased & updated for #10500

@findepi
Copy link
Member Author

findepi commented Jun 17, 2024

@nastra if this helps, i can split this PR into smaller ones, eg by module.

@nastra
Copy link
Contributor

nastra commented Jun 18, 2024

@findepi yeah I think it would make sense to split out the Flink + Spark changes. Everything else can stay in this PR

This was referenced Jun 18, 2024
Merged
Merged
@findepi
Copy link
Member Author

findepi commented Jun 18, 2024

@nastra no problem, extracted #10531 and #10532.
I kept those commits here for now, since the last commit adds static code analysis and otherwise wouldn't pass.

@nastra
Copy link
Contributor

nastra commented Jun 18, 2024

This can be rebased now, since the other 2 PRs have been merged

@findepi
Require Assertions to be statically imported
b492449 
Per code style recommendations.
@findepi
Copy link
Member Author

findepi commented Jun 18, 2024

rebased

nastra
nastra approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for cleaning this up

@nastra nastra merged commit 5ea78e3 into apache:main Jun 18, 2024
42 checks passed
@findepi findepi deleted the findepi/asserts branch June 18, 2024 11:01
@findepi
Copy link
Member Author

findepi commented Jun 18, 2024

thanks for your review @nastra !

jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024
@findepi @jasonf20
Statically import methods from AssertJ Assertions (apache#10517)
fe05cc1
@nastra nastra mentioned this pull request Aug 8, 2024
Merged
sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
@findepi
Statically import methods from AssertJ Assertions (apache#10517)
f860fdc
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@findepi @zachdisc
Statically import methods from AssertJ Assertions (apache#10517)
c019981
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nastra nastra nastra approved these changes

Assignees
No one assigned
Labels
ALIYUN API arrow AWS AZURE core data DELL flink GCP INFRA MR NESSIE ORC SNOWFLAKE spark
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @nastra