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

A bit of refactoring in the data engine, to improve type safety #1531

Merged
merged 1 commit into from Jan 9, 2024

Conversation

hvbtup
Copy link
Contributor

@hvbtup hvbtup commented Jan 1, 2024

There are no functional changes.
Added types to many collections.
Removed unnecessary casts.
Removed unnecessary SuppressWarnings tags.
Removed the class PropertySecurity, because it just routed to System.getProperty.
Simplified some if-else constructs as suggested from the IDE.

All of this in an attempt to make the code at least a little bit more comprehensible.

@hvbtup
Copy link
Contributor Author

hvbtup commented Jan 2, 2024

I see that the commit message is wrong. In fact I started with the intention to fix endless JDBC ParameterMetaData log messages, but then I saw that the code used untyped collections everywhere. So I began to add types to the collections. I commited, and later continued with the typing work.
Finally I came to the conclusion that a PR should not deal with more than one topic, so I removed the changes regarding JDBC ParameterMetaData log messages again.

Copy link
Contributor

@speckyspooky speckyspooky left a comment

Choose a reason for hiding this comment

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

I have checked the replaces and the changes to the typed collections looks good.

@hvbtup
Copy link
Contributor Author

hvbtup commented Jan 9, 2024

@wimjongman Can you take a look at the code?
I have to say that I'm not an expert on typed collections in Java.
Is there any risk that code outside of the BIRT project (eg custom plugins) will break after this change, would such custom code need a recompilation, would this recompilation result in errors?
Probably nobody cares about a few warnings more or less, but if this would be a breaking change for plugin developers we should know it.
Do you know anybody who actually uses or develops custom plugins (for extend items)? Because I guess that would be the only place where this PR could possibly have a negative effect, so these people should test.

@wimjongman
Copy link
Contributor

Sorry, I have no time to take a proper look at this.

@merks
Copy link
Contributor

merks commented Jan 9, 2024

@hvbtup I think this generally looks fine. Downstream clients would be using mostly raw types at this point so mostly such changes would have no impact because the erasures stay the same...

@hvbtup hvbtup merged commit bb4fc62 into eclipse-birt:master Jan 9, 2024
3 checks passed
@speckyspooky speckyspooky added this to the 4.15 milestone Jan 9, 2024
@speckyspooky speckyspooky added the Enhancement Small change to improve the current supported functionality label Jan 9, 2024
@wimjongman
Copy link
Contributor

Thanks, Ed!

hvbtup added a commit to triestram-partner/birt that referenced this pull request Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Small change to improve the current supported functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants