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

Remove sim checkstyle suppressions #2714

Closed
wants to merge 29 commits into from

Conversation

Starlight220
Copy link
Member

@Starlight220 Starlight220 commented Sep 15, 2020

Fixes #1290

  • add comment about saving the CallbackStore object so GC doesn't cancel the callback.

@Starlight220 Starlight220 requested a review from a team as a code owner September 15, 2020 06:30
@Starlight220
Copy link
Member Author

Starlight220 commented Sep 15, 2020

@AustinShalit - Most of the checkstyle errors are AbbreviationAsWordInName for FGPA or RIO. What should I do with them?
And what are the guidelines for wrapping Java param lists?
And what should be the javadoc for the SimValueInfo ctor?

@AustinShalit
Copy link
Member

AustinShalit commented Sep 15, 2020

I would leave AbbreviationAsWordInName for now. Just apply the others in this PR

I am not exactly sure. I think @PeterJohnson might have a better answer than what I can give.

@PeterJohnson
Copy link
Member

Both SimValueInfo and SimDeviceInfo (earlier in the same file) are structures that contain information about the simulated value and simulated device as returned by the corresponding Enumerate functions. Those constructors are only called from the native side and are not intended to be called by users.

@Starlight220
Copy link
Member Author

So should I @SuppressWarnings("JavadocMethod") it? And what about the AbbreviationAsWordInName errors?

@Starlight220 Starlight220 requested a review from a team as a code owner September 16, 2020 05:02
@Starlight220
Copy link
Member Author

I'm just putting @SuppressWarnings() on almost all of the errors...

@Starlight220
Copy link
Member Author

Starlight220 commented Sep 16, 2020

@PeterJohnson - what should I do with missing Javadocs on CallbackStore.java? And I'm suppressing NoFinalizer on that file. I'm also suppressing any naming issues (ParameterName, etc).

  • Javadoc on Field2d ctor missing
  • GenericHIDSim, JoystickSim, SimDeviceSim, XBoxController need Javadocs

Many of the needed Javadocs are for get/set/callback sets. I can write the get/set Javadocs as "<Gets/Sets> the property of the simulated object.". Would that be enough?

@Starlight220
Copy link
Member Author

It's waiting only on missing Javadoc errors.

@PeterJohnson
Copy link
Member

Yep, for get/set/callback you can just use a generic description.

CallbackStore keeps track of a simulation callback and cancels the callback when close() is called. The different constructors are for index-based, channel-based, or general (no index) callbacks.

Field2d constructor simply creates a Field2d object. There's no args to discuss.

@Starlight220
Copy link
Member Author

Starlight220 commented Oct 20, 2020

public void setAxisType(int axis, int type) {

I think that this type int should be some sort of enum... Or at least docs as to the different values possible

And in all the registerXxxCallback functions I'm assuming that the initialNotify parameter decides if the callback is called with the initial value.

The CallbackStore ctors should only be accessed from the XxxSim classes, right? They aren't for team usage?

@PeterJohnson
Copy link
Member

It’s the same type as Joystick.getAxisType returns, so we probably need to document both.

You are correct on what initialNotify does.

Yeah, CallbackStore is an implementation detail; teams generally shouldn’t need to use it directly.

@Starlight220
Copy link
Member Author

What's up with the CI?

@AustinShalit
Copy link
Member

What's up with the CI?

GitHub was having issues earlier today. I am going to rerun and see if that helps.

@Starlight220
Copy link
Member Author

Watchdog tests failing? Again?

@calcmogul
Copy link
Member

That's a known issue.

@PeterJohnson
Copy link
Member

I cleaned this up as #3079.

@Starlight220 Starlight220 deleted the patch-1 branch January 12, 2021 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable all sim checkstyle checks
4 participants