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

Documentation generation for custom types #847

Merged
merged 18 commits into from Mar 25, 2024
Merged

Conversation

ltabis
Copy link
Contributor

@ltabis ltabis commented Mar 20, 2024

Add documentation for types that derive the CustomType macro with the metadata feature.

@schungx
Copy link
Collaborator

schungx commented Mar 21, 2024

Still too many _with_comments for my comfort... I went down this route before and it took a lot of breaking changes and deprecation to finally move them all into the FuncRegistration API... I would hesitate to add a whole bunch of _with_comments yet again.

I am wondering if there is a way to do this without introducing a parallel set of API's...

How about if we include "the last function hash" inside TypeBuilder. Then you can do:

builder
    .with_get("foo", ...)      // stores hash in builder type
    .add_comments(...)     // always uses hash in builder (if Some) to look up function in engine
    ....

The global namespace can be accessed via Engine::global_namespace_mut. We can then add a crate-only method to Module, say add_fn_comments that can just lookup the function by hash, and then update the comments.

If you don't mind updating the parameter and return type names also, you can just use Module::update_metadata_with_comments.

@ltabis
Copy link
Contributor Author

ltabis commented Mar 22, 2024

@schungx I think this is a good idea ... for functions. I guess that for types we still need a with_name_and_comments function since the engine does not store hashes for them.

@schungx
Copy link
Collaborator

schungx commented Mar 22, 2024

Since each builder has only one type, it is quite trivial to add a with_comments for the builder.

Implementation wise, it can lookup the registered type via TypeId and add comments to it.

This way no public API needs to change. Only a few more methods to the builder API.

@ltabis
Copy link
Contributor Author

ltabis commented Mar 22, 2024

I have done the changes, however when I execute the custom_types_and_methods example the function parameters and return type information seems lost. I'm looking for the cause of the problem.

@ltabis
Copy link
Contributor Author

ltabis commented Mar 22, 2024

also rustc 1.77 seems to break the ui_tests/rhai_mod_inner_cfg_false.rs test

@ltabis
Copy link
Contributor Author

ltabis commented Mar 22, 2024

I have done the changes, however when I execute the custom_types_and_methods example the function parameters and return type information seems lost. I'm looking for the cause of the problem.

Find out why: using the FuncRegistration API directly does not set the params_info by itself.
Fixed it by moving the information filling directly in FuncRegistration::register_into_engine. (since it is called anyways in Engine::register_fn)

@schungx
Copy link
Collaborator

schungx commented Mar 23, 2024

Looking good!

A couple of observations:

  • Since an empty Vec indicates no hashes, you can omit the Option

  • Probably not a good idea to move the default params info into register_into_engine... What if the user used with_params_info before? Those would be overwritten...

Maybe...

#[cfg(feature = "metadata")]
self = if self.metadata.params_info.is_none() {
    ...
    self.with_params_info(...)
} else {
    self
};

self......

@ltabis
Copy link
Contributor Author

ltabis commented Mar 24, 2024

A couple of observations

I agree with your comments, I made the changes.

@schungx
Copy link
Collaborator

schungx commented Mar 25, 2024

I think it looks good. Let's merge this.

@schungx schungx merged commit 6619595 into rhaiscript:main Mar 25, 2024
37 of 39 checks passed
@schungx
Copy link
Collaborator

schungx commented Mar 25, 2024

I wonder if it is possible to avoid adding update_fn_comments...

Since the builder API is in the same crate, I think I can just inline the code in.

EDIT: I ended up replacing update_fn_comments with the potentially more useful get_fn_metadata_mut which allows you to change other fields.

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.

None yet

2 participants