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

SmartDashboard.setDefault* documentation #6490

Merged
merged 3 commits into from
Dec 30, 2024

Conversation

chauser
Copy link
Contributor

@chauser chauser commented Mar 30, 2024

I would like to address #6334, but realized that some discussion about how the behavior should be documented would help before changing all of them. The code diff here contains a proposal that reflects my understanding of the actual behavior.

Questions:

  1. Would you suggest different wording for documenting the behavior of these methods?
  2. Should documentation in NetworkTableEntry.java, NetworkTablesJNI.java, ntcore_c.h, and ntcore_cpp.h also be addressed in the same PR, or in a second PR specific to the ntcore component?

More details about NetworkTableEntry.java, NetworkTablesJNI.java, ntcore_c.h, and ntcore_cpp.h:

SetDefault* methods in NetworkTableEntry.java called by SmartDashboard.setDefault* document the return value as @return False if the entry exists with a different type whereas the actual behavior is @return False if the entry exists. Interestingly, the descriptive documentation here is Sets the entry's value if it does not exist. which is correct.

The next level down is NetworkTablesJNI.java. Here the documentation is

/**
   * Sets default topic value.
   *
   * @param entry Entry handle.
   * @param time Time in microseconds.
   * @param defaultValue Default value.
   * @return True if set succeeded.
   */

which is correct but doesn't identify criteria for success.

On the C/C++ side:

ntcore_c.h has

/**
 * Set Default Entry Value.
 *
 * Returns copy of current entry value if it exists.
 * Otherwise, sets passed in value, and returns set value.
 * Note that one of the type options is "unassigned".
 *
 * @param entry     entry handle
 * @param default_value     value to be set if name does not exist
 * @return 0 on error (value not set), 1 on success
 */

having both the "Returns a copy of..." error and lack of specificity about success. ntcore_cpp.h has essentially the same issues as ntcore_c.h.

@chauser chauser requested a review from a team as a code owner March 30, 2024 05:39
@chauser chauser marked this pull request as draft March 30, 2024 05:41
@chauser chauser marked this pull request as ready for review March 30, 2024 17:55
@chauser
Copy link
Contributor Author

chauser commented Apr 1, 2024

/format

@chauser chauser requested review from PeterJohnson and a team as code owners May 30, 2024 06:40
@chauser chauser force-pushed the SmartDashboardSetDefault branch from 7ee6a8a to c0a8d08 Compare May 30, 2024 21:22
rzblue
rzblue previously requested changes Dec 16, 2024
Copy link
Member

@rzblue rzblue left a comment

Choose a reason for hiding this comment

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

SmartDashboard.h needs to be fixed similar to java everywhere it says this:

Gets the current value in the table, setting it if it does not exist.

* @param defaultValue the default value to set if key does not exist.
* @return False if the table key exists with a different type
* @param defaultValue the value to set if key does not exist
* @return True: success; False the key already existed
Copy link
Member

@rzblue rzblue Dec 16, 2024

Choose a reason for hiding this comment

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

Suggested change
* @return True: success; False the key already existed
* @return True if the key did not already exist, otherwise false

ditto for all others for consistency

@rzblue
Copy link
Member

rzblue commented Dec 16, 2024

Should documentation in NetworkTableEntry.java, NetworkTablesJNI.java, ntcore_c.h, and ntcore_cpp.h also be addressed in the same PR, or in a second PR specific to the ntcore component?

Addressing them in the same PR would be fine

Would you suggest different wording for documenting the behavior of these methods?

I think an appropriate wording would be

Sets the entry's (or "topic", as appropriate) value if it does not exist. Returns true if the value did not already exist, otherwise false.

@chauser chauser force-pushed the SmartDashboardSetDefault branch from c0a8d08 to 13024a7 Compare December 16, 2024 22:50
@github-actions github-actions bot added component: ntcore NetworkTables library component: wpilibj WPILib Java component: wpilibc WPILib C++ labels Dec 16, 2024
@chauser
Copy link
Contributor Author

chauser commented Dec 16, 2024

Still WIP

… also fix description of defaultValue parameter for some Get* methods where the comment said that the value in the table would be set by the Get method
@chauser chauser force-pushed the SmartDashboardSetDefault branch from 13024a7 to 9ffb250 Compare December 27, 2024 00:21
@chauser
Copy link
Contributor Author

chauser commented Dec 27, 2024

I think this is ready for re-review. While I was fixing SmartDashboard.h I noticed that the Get* methods also were erroneous in saying that their defaultValue would be "set" if the key was not in the table, so I fixed those too.

@chauser chauser requested a review from rzblue December 27, 2024 00:47
@PeterJohnson PeterJohnson merged commit eef1bf3 into wpilibsuite:main Dec 30, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ntcore NetworkTables library component: wpilibc WPILib C++ component: wpilibj WPILib Java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants