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

distinguish regular modules from module types #925

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

zth
Copy link
Collaborator

@zth zth commented Feb 25, 2024

Fixes #902

@woeps please look through this and say what you think. A few things I can think of right away:

  • We don't indicate when a module is implementing a module type. Not sure if this would be relevant for docgen.
  • We can have id clashes now I guess, because I think a regular module and a module type with the same name can co-exist in the same module scope. So we need to account for that in the id generation.

Copy link
Contributor

@woeps woeps left a comment

Choose a reason for hiding this comment

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

Wow, you were fast!

  • What about adding the signature field to the Module specific records (like in non-module related tools.docItem constructors? Have it show e.g. "signature": "module M: Example" or for modules not using a previously defined module type "signature": "module AnotherModule: { type t; let x: string; ...".
  • Do you refer to @aspeddro s work? He introduced the source record {filepath: string, line: int, col: int}, which should be different in your described case.
  • Ideally, I'd like to have a source record also available for the refered module type. The record type tools.docsForModule includes a source field, which describes, where the module was specified. But if the module uses a module type, there is currently no source for that module type. Having such info would be very handy for linking up the different files in a site generator.
  • Am I right to assume, these changes do not affect the language server's auto-complete suggestions? - I only tested tools locally.

"line": 109,
"col": 3
}
}]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also expect "id": "DocExtractionRes.M" to be present. - Which is missing here?

"id": "DocExtractionRes.Example",
"name": "Example",
"kind": "moduleType",
"docstrings": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

docstrings shouldn't be empty, since the module type contains:

module type Example = {
  /***
  this is an example module type 
  */
...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe /*** is for the top level only. If you want a comment on an in-file module you need to use /** just above the module definition, like regular docstrings.

Copy link
Contributor

@woeps woeps Mar 1, 2024

Choose a reason for hiding this comment

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

You confused me for a minute there.. ;)
I don't think so.
See my simple example:
https://github.com/woeps/rescript-tools-doc-md/blob/54b93c3da527074be767227c057e5b10dbdf7b85/test/ExampleModule.res#L6

https://github.com/woeps/rescript-tools-doc-md/blob/54b93c3da527074be767227c057e5b10dbdf7b85/test/md/Example.md?plain=1#L166

The difference in my example is the usage inside a submodule. - but not a module type.

@woeps
Copy link
Contributor

woeps commented May 31, 2024

Any further updates/thoughts on this one?

@zth
Copy link
Collaborator Author

zth commented May 31, 2024

@woeps not really, but I'd be happy to try and fix it quickly if I understood exactly what needs to be done. Will try and re-read the instructions.

@woeps
Copy link
Contributor

woeps commented Jun 6, 2024

@zth I previously created several comments in this PR regarding overall expectations of docgen.
The PR does, what's stated in the title and actually does not need (should not) address my regards.

I'd suggest to merge this as is and I'll create follow up issues for next steps. (e.g. id-ambiguity)

@zth
Copy link
Collaborator Author

zth commented Jun 7, 2024

@woeps ok, merging this and we'll continue in separate issues for the other things. Please open new issues as you see fit.

@zth zth merged commit f066b35 into master Jun 7, 2024
5 checks passed
@zth zth deleted the fix-module-type-in-docgen branch June 7, 2024 06:08
jfrolich pushed a commit to jfrolich/rescript-vscode that referenced this pull request Sep 3, 2024
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.

handling of module types (in doc extraction)
2 participants