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

stop rendering overridden module vals #580

Merged

Conversation

lubegasimon
Copy link
Collaborator

@lubegasimon lubegasimon commented Feb 3, 2021

Fixes #508

after fix:

Screenshot 2021-02-03 at 14 30 54

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Most of the CI failures seems to come from Mdx, can you try rebasing on top of #591 ?

src/model/ident_env.cppo.ml Outdated Show resolved Hide resolved
src/model/ident_env.cppo.ml Outdated Show resolved Hide resolved
src/model/names.ml Outdated Show resolved Hide resolved
@lubegasimon
Copy link
Collaborator Author

Most of the CI failures seems to come from Mdx, can you try rebasing on top of #591 ?

It happens, that this has to be merged first.

@Julow
Copy link
Collaborator

Julow commented Feb 16, 2021

You can import these changes in this PR like this:

git fetch -f origin pull/591/head
git rebase FETCH_HEAD

Git will be smart enough to remove them from your branch when #591 is merged and you rebase again on top of master

Signed-off-by: lubegasimon <lubegasimon73@gmail.com>
@lubegasimon lubegasimon force-pushed the stop-documenting-overridden-module-vals branch 2 times, most recently from 0a44168 to c77755f Compare February 19, 2021 17:17
Signed-off-by: lubegasimon <lubegasimon73@gmail.com>
@lubegasimon lubegasimon force-pushed the stop-documenting-overridden-module-vals branch from c77755f to 4589202 Compare February 19, 2021 17:20
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Looks good !

src/model/ident_env.cppo.ml Outdated Show resolved Hide resolved
@@ -275,7 +313,7 @@ let extract_structure_tree_item item =
| Tstr_open o ->
((extract_extended_open o) :> extracted_items list)
#endif
| Tstr_eval _ | Tstr_value _
| Tstr_eval _
| Tstr_primitive _ | Tstr_typext _
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tstr_primitive binds a value too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This statement isn’t clear

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tstr_primitive correspond to:

external f : unit -> unit = "%primitive_name"

It should be treated as a value (as val f : unit -> unit)

src/model/names.ml Outdated Show resolved Hide resolved
Signed-off-by: lubegasimon <lubegasimon73@gmail.com>
@lubegasimon lubegasimon force-pushed the stop-documenting-overridden-module-vals branch from 15a350d to d40779f Compare February 21, 2021 10:41
@jonludlam jonludlam merged commit 401f26e into ocaml:master Feb 22, 2021
@lubegasimon lubegasimon deleted the stop-documenting-overridden-module-vals branch February 22, 2021 18:18
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.

overridden values of modules included in another module still get rendered
3 participants