-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
[BUGFIX] Display external pages on all levels of the main menu #900
Conversation
fae8c21
to
2a37a55
Compare
I will have to review this more in depth, but it looks like there are a number of breaking changes in this pr. Will come back to this later |
All places that use now DocumentEntryNode|ExternalEntryNode before used DocumentEntryNode so it shouldn't really be breaking. Also as the Transformers are final they should not have been extended so we do not need to worry there. I introduced a new level of inheritence in Nodes, however now the DocumentEntryNode extends the EntryNode which extends the AbstractNode, before it extended the AbstractNode directly. This should also be not breaking. There is a slight possibility that someone expects all children of DocumentEntryNode to be DocumentEntryNode. And now this only returns an EntryNode. We could instead of altering getChildren introduce a new method and let getChildren filter only for DocumentEntryNodes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, we just released 1.0.0. So I do not expect anyone to use this functionality in to much depth, and the breaking change is limited. But I would like to get some awareness around the things that have been changed.
Stretching the arguments of methods is ok, people using the methods directly will not be impacted as we still accept the same types as before. The changes in the compiler passes should also not really impact people.
Changing return types is a no go, as I explained in the review comments. I would like to be constructive in this and not to strict. But I also want to ensure that we are all strict on these type of changes.
2a37a55
to
63443df
Compare
@jaapio I have some questions, see above |
63443df
to
1f8dba0
Compare
Sphinx handels it also that way
85f54e7
to
a4b8b28
Compare
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
[1.x] Merge pull request #900 from phpDocumentor/task/menu-external
Sphinx handels it also that way