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 documentation options for extension!() #232

Merged
merged 24 commits into from
Sep 26, 2023
Merged

Add documentation options for extension!() #232

merged 24 commits into from
Sep 26, 2023

Conversation

rscarson
Copy link
Contributor

Added docs: comma separated list of toplevel #[doc=...] tags to be applied to the extension's resulting struct to extension! to allow users to set docblocks for it, since the struct is always made public

Added comments for generated functions, since they break linting when warning about missing function docs

Added an op2 example that uses the change (also it is difficult to find samples of op2 and extension! in use with the runtime - having access to such an example would have simplified my usage of the crate significantly

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2023

CLA assistant check
All committers have signed the CLA.

@rscarson rscarson changed the title Add documentation options for extension! Add documentation options for extension!() Sep 22, 2023
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I think this will be very useful. Let's tweak the comments a bit to make them.

core/examples/op2.rs Outdated Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
core/examples/op2.rs Outdated Show resolved Hide resolved
rscarson and others added 6 commits September 24, 2023 22:34
Comment on lines 376 to 381
/// Initialize this extension for use with CommonJS
/// For modules, use init_ops_and_esm instead
///
/// # Returns
/// an Extension object that can be used during instantiation of a JsRuntime
pub fn init_js_only $( < $( $param : $type + 'static ),* > )? () -> $crate::Extension
Copy link
Collaborator

@nayeemrmn nayeemrmn Sep 26, 2023

Choose a reason for hiding this comment

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

I think we should leave this one undocumented, or even mark it as deprecated. We don't use it in CLI or anywhere I know of. And I don't know when you'd use it. It might be a legacy thing. It seems to just not execute the state fn and middleware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

      /// Legacy function for extension instantiation.
      /// Please use `init_ops_and_esm` or `init_ops` instead
      ///
      /// # Returns
      /// an Extension object that can be used during instantiation of a JsRuntime
      #[deprecated(since="0.216.0", note="please use `init_ops_and_esm` or `init_ops` instead")]

Added deprecation notice, and altered documentation to reflect the change

core/extensions.rs Outdated Show resolved Hide resolved
core/extensions.rs Outdated Show resolved Hide resolved
rscarson and others added 3 commits September 26, 2023 16:15
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

LGTM

@mmastrac mmastrac enabled auto-merge (squash) September 26, 2023 20:25
@mmastrac mmastrac merged commit d9faead into denoland:main Sep 26, 2023
4 checks passed
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.

4 participants