Fix public facing #4
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Defined structs for use by setup_complete and sdk_info to simplify the implementation and better document what these JSON messages actually contain. - Defined structs for location and antithesis_assert to simplify the implementation and better document what these JSON messages actually contain. - Update LibHandler::output to accept (message &str) which is intended to be JSON formatted UTF-8 text. - General source cleanup and removal of debug/trace print statements - Use AssertType directly rather than represent it as a text string - Add comments to the assert_helper macro to clarify its implmentation - Improve the interface to the assert_raw() function so that ownership of its parameters will be transferred to assert_raw(). The exception is the `details` parameter which is always passed by borrowed reference.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[X] setup_complete - make a struct for the data, and then json-ify it here
[X] Same for sdk_info function in internal
[X] LibHandler::output feels wrong to me. It feels like it should either take a string (&str) or it should take an object that is Serializable and do the serialization itself. With appropriate adjustments to callers. Or possibly the subclasses should implement the &str version and the trait should have the method that deals with &Value and with Serializable types
[X] For LocalHandler, would it make more sense to have a true local handler and a no-op handler, and have the new return the appropriate one?
[X] assert/mod.rs - line 32 has a debug println!
[X] In CatalogInfo and AssertionInfo, why is assert_type a &str and a String? Why not an AssertType? (prefer the typed version)
[X] Make typed classes for location and antithesis_assert. Use Serde to serialize where needed.
[X] In assert_helper, maybe add some comments about what some of the top-level things are doing. For example, are we putting the whole thing in {}? Mention that? And the NAME and FUN_NAME - give an example of what those outputs look like; that kind of thing.
[X] How did you solve the unreachable namespace collision?
Using these macro names: assert_always! assert_sometimes! assert_always_or_unreachable!, assert_reachable!, assert_unreachable!
[X] Is there a place I can see the rendered rust docs?
afaik the docs are not public yet. For now, checkout the 'main' branch of the repo on a system that has cargo available, and use
cargo doc --open[X] I would tend to make AssertionInfo not clone arguments in new, but rather take arguments by ownership. Then callers could clone or not clone depending on their needs. The idea would be that if someone has a string they don't need any more, they can just pass it in and we won't need to do an unnecessary clone (String -> &str -> String).