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.

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

Use uniform Assertions AssertJ class #19013

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

ksobolew
Copy link
Contributor

Description

According to javadocs, the AssertionsFor(Class|Interface)Types were introduced to circumvent type inference issues with generic classes in Java 8. But it seems that we don't have these issues in our code base, so it looks like importing from these classes was just a mistake.

Additional context and related issues

These classes also have somewhat impoverished API. That's actually how I found it - I couldn't find a method I was expecting in one place.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Sep 12, 2023
@github-actions github-actions bot added tests:hive iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector labels Sep 12, 2023
@wendigo
Copy link
Contributor

wendigo commented Sep 12, 2023

I've killed a build, since master is broken due to compilation issue: #19014

@ksobolew
Copy link
Contributor Author

Thanks, I'll rebase once the fix is merged.

@ksobolew
Copy link
Contributor Author

Although funny thing is, it compiled for me locally?

@wendigo
Copy link
Contributor

wendigo commented Sep 12, 2023

Did you rebase on master?

@ksobolew
Copy link
Contributor Author

No, and this branch doesn't even include the faulty commit 🤔

@ksobolew
Copy link
Contributor Author

Anyway, the fix is in so I'll just rebase.

According to javadocs, the `AssertionsFor(Class|Interface)Types` were
introduced to circumvent type inference issues with generic classes in
Java 8. But it seems that we don't have these issues in our code base,
so it looks like importing from these classes was just a mistake.
@wendigo
Copy link
Contributor

wendigo commented Sep 13, 2023

@ksobolew github always rebased on target branch when running CI

@ksobolew
Copy link
Contributor Author

I see...

@ksobolew ksobolew marked this pull request as draft September 13, 2023 10:20
@ksobolew
Copy link
Contributor Author

I want to add these classes to the banned imports list, but I'll need a fix in airbase first, so I'll make it a draft for now.

@wendigo
Copy link
Contributor

wendigo commented Sep 13, 2023

Is this blocking this change from being merged @ksobolew ?

@ksobolew
Copy link
Contributor Author

Nope, restrict-imports-enforcer-rule is broken and it's impossible to add additional RestrictImports rules in a child POM.

  • All regular rules require only a single declaration; if multiple are present, only the first one is used
  • But RestrictImports requires multiple declarations so that we can group them and assign different <reason>s for them
    • There's a possibility to use <group><group>...</group>...</groups>, but the groups are exclusive - only the one with the most-specific base package is chosen, the rest is ignored - while multiple RestrictImports are inclusive
  • If we add combine.children="append" to the parent <rules> tag, we can add additional <RestrictImports> declarations, but everything else is broken, because the child POM adds additional declaration instead of merging with the parent one, so it breaks on the first bullet point above
  • No other combination of combine.self="..." seems to make it work for <RestrictImports> and not for other rules

Note: there is one place in trino-base-jdbc where we do this (<rules combine.children="append">), but we don't risk breaking other rules there, so it's fine there. But it's not fine to do that for all modules.

@ksobolew ksobolew marked this pull request as ready for review September 13, 2023 11:44
@ksobolew
Copy link
Contributor Author

Is this blocking this change from being merged @ksobolew ?

No, not blocking. I was just hoping to do it in one PR. But it can be merged as-is.

@wendigo
Copy link
Contributor

wendigo commented Sep 13, 2023

@ksobolew we should add this rule to airbase

@wendigo
Copy link
Contributor

wendigo commented Sep 13, 2023

@ksobolew do you want this change to be merged?

@ksobolew
Copy link
Contributor Author

Yes, please :)

@ksobolew
Copy link
Contributor Author

@ksobolew we should add this rule to airbase

👍

@wendigo wendigo merged commit 83d9c6e into trinodb:master Sep 13, 2023
93 checks passed
@ksobolew ksobolew deleted the kudi/assertj-assertions-class branch September 13, 2023 11:54
@ksobolew
Copy link
Contributor Author

airlift/airbase#376

@github-actions github-actions bot added this to the 427 milestone Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector
Development

Successfully merging this pull request may close these issues.

3 participants