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

Cleaned up test code to fix all test related spotbugs warnings #2006

Merged
merged 13 commits into from Sep 20, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 14, 2023

This is a step towards #1806. Fixes are based on warnings from spotbugs and IntelliJ Idea.

This builds on #2005 and should only be merged after.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 15, 2023

@a-sr, @soerendomroes I tried to add the dependencies on com.github.spotbugs:spotbugs-annotations and net.jcip:jcip-annotations to the Epoch maven configuration, but I was not able to get it to work. I suspect this also needs to be added to the target platform. But how? Could you help with this?

@a-sr
Copy link
Collaborator

a-sr commented Sep 20, 2023

lf-lang/epoch#54 adds the jcip and spotbugs dependency to the target platform and the LF plugin.
The PR will not update the LF subrepo. I don't know what is the best strategy in this case.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 20, 2023

Thanks to Alex, the Epoch build is now fixed. As far as I am concerned, this is ready to be merged.

@cmnrd cmnrd added the cleanup label Sep 20, 2023
@cmnrd cmnrd changed the title Clean up of test code and fix all test related spotbugs warnings Cleaned up test code to fix all test related spotbugs warnings Sep 20, 2023
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This looks good to me! The usage of StandardCharsets.UTF_8 seems a bit verbose to me; is that really the way proper Java ought to be written these days?

@cmnrd cmnrd added this pull request to the merge queue Sep 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 20, 2023
@cmnrd cmnrd added this pull request to the merge queue Sep 20, 2023
@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 20, 2023

Yeah, I am not sure about those, actually. The problem is, that there is no well-defined default. So if you write a file and then read it back in on another machine, you may end up with garbage. This is why it is better to define the encoding we actually want. However, in our case, we don't really know the encoding used in the pipes to external processes. So maybe it would be safer to use the default... but I don't know, and I figured we could try if using UTF-8 throughout causes any troubles.

Merged via the queue into master with commit 7284ddd Sep 20, 2023
41 checks passed
@cmnrd cmnrd deleted the spotbugs-fixes branch September 20, 2023 18:51
cmnrd added a commit that referenced this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants