-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Issue #532: Implement get_path helper API #559
base: main
Are you sure you want to change the base?
Conversation
@ujsquared thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment:
To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
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.
Some reStructuredText syntax and English grammar docs and change log fixes. Otherwise the docs and change log look good to me.
Links to rendered docs to review.
- https://ploneapi--559.org.readthedocs.build/content.html#get-content-path
- https://ploneapi--559.org.readthedocs.build/api/content.html#plone.api.content.get_path
- https://ploneapi--559.org.readthedocs.build/_modules/plone/api/content.html#get_path
Still needs a technical review. It looks pretty good from what I can understand.
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.
Mostly opinions, let's see if others agree on them
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.
Docs LGTM! Technical review still needed.
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.
IMO it is disorienting that a relative path starts with "/".
I find it awkward but acceptable that absolute_url(relative=True)
returns an initial "/" (the method has absolute
in its name), but get_path(obj, realtive=True)
returning something with a leading slash feels inconsistent.
okay, will fix this. |
@ale-rt @ujsquared I was going back and forth about whether I like removing the slash from the portal-relative path, but it has a nice benefit that you can do portal.unrestrictedTraverse(path) with the result. |
You mean that it is better without the slash:
Note that you do not need to pass a string to resolve a path:
|
Currently, the repo has more instances of using |
…d/plone.api into issue-532-add-get_path
I would say:
So:
So for the absolute case this would be: |
Excatly:
|
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.
Minor changes. Thanks for the contribution and responsiveness to suggestions.
Direct links to affected docs pull request preview pages all LGTM.
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.
I would still much prefer that we call the parameter Never mind, I looked again at Gil's reason of making it match relative_to_portal
instead of relative
. I think it makes it clearer what it does.absolute_url
, and I think that makes sense.
@jenkins-plone-org please run jobs |
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.
Docs look good to me. Thank you! Are there any unresolved matters to address?
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.
LGTM
Fixes Issue #532
relative_to_portal
to fetch patch relative to Plone portalInvalidParameterError
when used to fetch an object outside of Plone portal withrelative_to_portal
parameter set to True📚 Documentation preview 📚: https://ploneapi--559.org.readthedocs.build/