zsmartsystems / com.zsmartsystems.zigbee

ZigBee Cluster Library Java framework supporting multiple dongles
Eclipse Public License 1.0
142 stars 88 forks source link

Fix most warnings #1396

Closed ViToni closed 1 year ago

ViToni commented 1 year ago

When using the Eclipse, the IDE reports a number of warnings about the workspace. The warnings are mostly of the types:

This PR fixes most of these warnings. Unfixed warnings remain for :

cdjackson commented 1 year ago

Thanks for this. It looks like you probably ran the autocoder over the auto generated code, which makes lots of changes that aren't really associated with this PR, and it means the next time someone runs one of the autocoders, there will again be an awful lot of unnecessary changes as the autocoder will change these back.

For automatically generated code, I think it's best to leave the code as generated to avoid this kind of see-sawing. I think it will also make this PR a lot smaller and much much easier to review as I think it will reduce the number of changed files down considerably from the 542 showing at the moment.

ViToni commented 1 year ago

The PR touches also the generator code, so that generated files should be more or less clean.

Just pushed one more commit to make the imports of generated files more selective. The diff between generated code and code already checked in, is mostly the @Generated annotation due to the timestamp. But that there are still some diffs due to unused imports being generated.

cdjackson commented 1 year ago

I don't think so - sorry.

As an example, and it's the first and only one I looked at, the EmberApsFrame changed the order of the imports in your commit. When I ran the Ember autocoder, it changed them back again. So I assume that something reformatted the code after the autocoder ran?

image

Maybe this isn't as widespread as I thought - I don't know as this is the first thing I looked at since it's quite close to the top of the list of files and it seemed strange that in these files the only changes were the order of commits. I looked in the autocoder thinking maybe you'd change the autocoder to reorder the commits and didn't see anything. So I grabbed your branch and ran the autocoder, and it changed these file back to the original.

cdjackson commented 1 year ago

I note that this still includes a lot of files that are not directly produced by the autocoder - it would really be good to resolve this as I can't merge as it is.

ViToni commented 1 year ago

Regarding the generator part the PR initially took care mostly only of deserializer types. Now it touches also how imports are added and printed. It also takes care of some imports required by Javadoc so that order is kept stable as it would be within Eclipse. The regular autocoder is now stable. However there still will be some diffs when using the custom autocoder. As far as I see the main reasons are:

If the PR might be merged later on, it might be an option to not squash the PR as the many commits touch very different aspects and have different reasons why they have been created which would get lost when merged to Fix most warnings.

cdjackson commented 1 year ago

It looks to me like there has been a reformat of the files after the autocoder has run - otherwise how do you explain that some imports move from the bottom to the top of the imports list? Certainly if I run the autocoder here from your branch, it gives a different result to your PR.

it might be an option to not squash the PR

i will almost certainly squash the PR - otherwise things get messy. You also have commits in this PR that have already been merged.

many commits touch very different aspects and have different reasons why they have been created

You really should create small PRs for each issue - not just cram everything into one PR. It makes things easier to review, and avoids loosing things. Just not squashing is not really a good way to manage a repository - sorry.

cdjackson commented 1 year ago

It seems that just the Ember files have been reformatted - these just happened to be the ones I looked at. So, I'd suggest to fix that, and to break this into separate PRs for each new feature or change, then we can look to get stuff merged :)

cdjackson commented 1 year ago

I assume this is now superseded by the other PRs that have been merged, so closing...