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

Cleanup implementation of --parent-id #1126

Merged
merged 9 commits into from
May 24, 2024
Merged

Conversation

EmileTrotignon
Copy link
Collaborator

This cleans up the implementation of --parent-id.

It is currently possible to declare a module as your parent id, which can lead to conflicts that are silently resolved by overwritting pages.

@@ -79,7 +85,7 @@ let resolve_parent_page resolver f =
in
let extract_parent = function
| { Paths.Identifier.iv = `Page _; _ } as container -> Ok container
| _ -> Error (`Msg "Specified parent is not a parent of this file")
| { Paths.Identifier.iv = `LeafPage _; _ } -> Error (`Msg "leaf page")
Copy link
Member

Choose a reason for hiding this comment

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

Does this error message ever get exposed to the user?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this is a leftover from debugging. The more exhaustive matching should be kept though.

Suggested change
| { Paths.Identifier.iv = `LeafPage _; _ } -> Error (`Msg "leaf page")
| { Paths.Identifier.iv = `LeafPage _; _ } -> Error (`Msg "Specified parent is not a parent of this file")

EmileTrotignon and others added 6 commits May 23, 2024 09:21
Co-authored-by: panglesd <peada@free.fr>
Co-authored-by: panglesd <peada@free.fr>
When there was `--child` options passed, but no `--parent` option, the children
were ignored.

Co-authored-by: panglesd <peada@free.fr>
Co-authored-by: panglesd <peada@free.fr>
Signed-off-by: Paul-Elliot <peada@free.fr>
@jonludlam
Copy link
Member

I'd prefer parents_children over siblings as when I first read this I assumed that siblings wouldn't include self. A minor nit, but worth changing I think.

@panglesd
Copy link
Collaborator

when I first read this I assumed that siblings wouldn't include self

Good point!
Note that before this PR, "parents_childrenvariables were namedchildrenwhich caused a lot of confusion withchildren`, the variables for our own children :)

@jonludlam
Copy link
Member

Heh, naming is hard :-)

@jonludlam
Copy link
Member

Option.map is 4.08+. We've talked about dropping support for older OCamls, but I'd rather do this in a separate PR than just because we happen to use a newer function.

@EmileTrotignon
Copy link
Collaborator Author

Only the changelog is left in the PR, but I am not sure what to do with it because this PR cleans up something that was done in a previous PR already.

@panglesd panglesd added the no changelog This pull request does not need a changelog entry label May 23, 2024
@jonludlam jonludlam merged commit 1c3bbf6 into ocaml:master May 24, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants