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

Add NetworkTableSource #646

Merged
merged 9 commits into from
Aug 27, 2016

Conversation

AustinShalit
Copy link
Member

@AustinShalit AustinShalit commented Jul 30, 2016

closes #363
closes #572
closes #635

@AustinShalit AustinShalit added this to the v2.0.0 milestone Jul 30, 2016

private void addListener() {
entryListenerFunctionUid = NetworkTablesJNI.addEntryListener(path, (uid, key, value, flags)
-> objectProperty.set(value),
Copy link
Member

Choose a reason for hiding this comment

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

Arguments should each be on their own line

@codecov-io
Copy link

codecov-io commented Jul 30, 2016

Current coverage is 58.43% (diff: 70.63%)

Merging #646 into master will increase coverage by 0.39%

@@             master       #646   diff @@
==========================================
  Files           196        198     +2   
  Lines          6165       6282   +117   
  Methods           0          0          
  Messages          0          0          
  Branches        561        569     +8   
==========================================
+ Hits           3578       3671    +93   
- Misses         2422       2442    +20   
- Partials        165        169     +4   

Sunburst

Powered by Codecov. Last update bbea07c...fc680f8

@SamCarlberg
Copy link
Member

Write tests for the network tables source

@AustinShalit
Copy link
Member Author

AustinShalit commented Aug 3, 2016

Does anyone know why edu.wpi.grip.core.sources.SourcesSanityTest > testNulls is failing?

* @param path The path of the object to get
*/
public NetworkReceiver(String path) {
checkArgument(!path.isEmpty(), "Name cannot be an empty string");
Copy link
Member

Choose a reason for hiding this comment

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

Should also add an explicit nullcheck with checkNotNull(path, ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha.... Thank you!

@AustinShalit
Copy link
Member Author

Tests randomly fail...

@JLLeitschuh
Copy link
Member

@SamCarlberg Can you help out Austin?

networkReceiver.close();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Newline at EOF

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@AustinShalit
Copy link
Member Author

So maybe the error was a fluke? I reran the travis tests 3 times and the build did not fail.

/**
* Provides a way to get a {@link Types Type} from a NetworkTable that GRIP is connected to.
*/
@XStreamAlias("grip:NetworkTableValue")
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be "grip:NetworkTableEntry"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@AustinShalit
Copy link
Member Author

AustinShalit commented Aug 9, 2016

I labeled this as ready for review because I have not seen the tests failing after merging master into this branch.

@JLLeitschuh
Copy link
Member

So what I really don't want is Network Tables initialized in tests that don't need it. That's why, up until this point I've kept network tables out of tests.
So, since we have DI, I'd love if network tables was not injected where it's not needed. Do you think you can do that?

@AustinShalit
Copy link
Member Author

ummmm..... no promises but I will give it a shot

# Conflicts:
#	ui/src/test/java/edu/wpi/grip/ui/pipeline/PipelineUITest.java
@AustinShalit
Copy link
Member Author

@JLLeitschuh I have no idea how to do this. If you could please lend a hand 😃

*/
public abstract class NetworkReceiver implements AutoCloseable {

protected final String path;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a protected field?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in person

@JLLeitschuh
Copy link
Member

JLLeitschuh commented Aug 27, 2016

@AustinShalit AustinShalit merged commit a9659d0 into WPIRoboticsProjects:master Aug 27, 2016
@AustinShalit AustinShalit deleted the ntSource branch August 27, 2016 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AddSourceView - activeDialog not set Add Network Tables Source
4 participants