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

Add get_parents and get_closed_parent functions #560

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

rohnsha0
Copy link

@rohnsha0 rohnsha0 commented Feb 19, 2025

closes #531


📚 Documentation preview 📚: https://ploneapi--560.org.readthedocs.build/

@rohnsha0 rohnsha0 requested a review from davisagli February 19, 2025 18:19
@mister-roboto
Copy link

@rohnsha0 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

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!

@stevepiercy
Copy link
Contributor

@rohnsha0 this PR will need tests and docs, per https://6.docs.plone.org/plone.api/contribute.html

@rohnsha0
Copy link
Author

@stevepiercy Ik, I'll do it today 😅

@rohnsha0 rohnsha0 requested a review from ale-rt February 20, 2025 13:12
@rohnsha0 rohnsha0 requested a review from stevepiercy February 20, 2025 14:11
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

You are not actually getting the "parents" but the ancestors.

@rohnsha0
Copy link
Author

You are not actually getting the "parents" but the ancestors.

Okay, so how would I get that!? I'm unsure of the same! @ale-rt

@ale-rt
Copy link
Member

ale-rt commented Feb 20, 2025

@rohnsha0 the tests are failing, you have already been suggested to work on your editing tool.

Before pushing, please:

  • be sure that pre-commit passes
  • be sure the test pass

Please read: https://6.docs.plone.org/plone.api/contribute.html

@rohnsha0 rohnsha0 requested a review from ale-rt February 20, 2025 17:32
@rohnsha0
Copy link
Author

not sure, why the tests are now failing!



@required_parameters("obj")
def get_parents(obj: None, predicate: None, interface: None):
Copy link
Member

Choose a reason for hiding this comment

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

You're using type hints here but you should set default values. That's why the tests are failing.

Suggested change
def get_parents(obj: None, predicate: None, interface: None):
def get_parents(obj=None, predicate=None, interface=None):

Copy link
Author

Choose a reason for hiding this comment

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

done with changes, yet the tests are failing!

Copy link
Member

Choose a reason for hiding this comment

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

@rohnsha0 The change fixed some of the tests. There are some other tests still failing. Did you look at them or run them locally?

Copy link
Author

Choose a reason for hiding this comment

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

I'm getting this:

Total: 478 tests, 4 failures, 4 errors and 1 skipped in 38.404 seconds.
test: exit 1 (40.95 seconds) /home/rohnsha0/code/plone/plone.api> zope-testrunner --all --test-path=/home/rohnsha0/code/plone/plone.api/src -s plone.api pid=4250
  test: FAIL code 1 (44.92=setup[3.97]+cmd[40.95] seconds)
  evaluation failed :( (45.14 seconds)
make: *** [Makefile:61: test] Error 1

by running make test

Copy link
Author

Choose a reason for hiding this comment

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

but unable to understand what it says! @davisagli

Copy link
Contributor

@stevepiercy stevepiercy Feb 21, 2025

Choose a reason for hiding this comment

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

Looking at the Makefile, make test invokes tox -e test from the virtual environment, which in turn is configured in tox.ini. Relevant information is in this stanza:

plone.api/tox.ini

Lines 106 to 141 in a876d70

[testenv:test]
description = run the distribution tests
use_develop = true
skip_install = false
constrain_package_deps = true
set_env =
ROBOT_BROWSER=headlesschrome
##
# Specify extra test environment variables in .meta.toml:
# [tox]
# test_environment_variables = """
# PIP_EXTRA_INDEX_URL=https://my-pypi.my-server.com/
# """
#
# Set constrain_package_deps .meta.toml:
# [tox]
# constrain_package_deps = "false"
##
deps =
zope.testrunner
-c https://dist.plone.org/release/6.0-dev/constraints.txt
##
# Specify additional deps in .meta.toml:
# [tox]
# test_deps_additional = "-esources/plonegovbr.portal_base[test]"
#
# Specify a custom constraints file in .meta.toml:
# [tox]
# constraints_file = "https://my-server.com/constraints.txt"
##
commands =
zope-testrunner --all --test-path={toxinidir}/src -s plone.api {posargs}
extras =
test

My questions are:

  1. Should we use the constraints file https://dist.plone.org/release/6.0-dev/constraints.txt? This question is for the maintainers of plone.api.
  2. Try running the command zope-testrunner --all --test-path={toxinidir}/src -s plone.api {posargs}, resolving the slugs for your system accordingly. See its docs at https://zopetestrunner.readthedocs.io/en/latest/.

Make can sometimes mask useful feedback, and in the past I've been able to run the commands it invokes to debug and control it better. I'm sorry I am not much help, but that's how I would start to figure it out. I know nothing about zope-testrunner other than it does what its name says.

Copy link
Author

Choose a reason for hiding this comment

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

Try running the command zope-testrunner --all --test-path={toxinidir}/src -s plone.api {posargs}, resolving the slugs for your system accordingly. See its docs at https://zopetestrunner.readthedocs.io/en/latest/.

okay now it doesnt run any tests, strangely

Copy link
Contributor

Choose a reason for hiding this comment

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

What command did you try? Is there any output?

I'm not sure where the slug {posargs} gets its key/value pair.

Also I see this in tox.ini:

set_env =
    ROBOT_BROWSER=headlesschrome

Which indicates you need to do the following before issuing the zope-testrunner command in the current terminal session.

export ROBOT_BROWSER=headlesschrome

Copy link
Author

@rohnsha0 rohnsha0 Feb 22, 2025

Choose a reason for hiding this comment

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

i already set the robot_browser to env

$ zope-testrunner --all --test-path=src -s plone.api

Total: 0 tests, 0 failures, 0 errors and 0 skipped in 0.000 seconds.

Copy link
Member

@davisagli davisagli Feb 22, 2025

Choose a reason for hiding this comment

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

@stevepiercy You're sending @rohnsha0 on a bit of a wild goose chase. It's possible to run zope-testrunner directly but I don't think there's anything wrong using make test to run the tests -- that was already showing the 4 failures and 4 errors that need a closer look. And these aren't Robot Framework tests so ROBOT_BROWSER is not relevant.

@rohnsha0 4 failures, 4 errors means you have to look further up in the output, find those specific tests that failed, study what went wrong, and adjust either the tests or the implementation to fix it. I saw at least some of the failing tests are in the documentation.

I can take a look myself and try to help, but it will probably take me some days to get to it, since I have a lot of things to review.

@rohnsha0 rohnsha0 requested a review from davisagli February 21, 2025 02:28
@stevepiercy
Copy link
Contributor

You are not actually getting the "parents" but the ancestors.

Okay, so how would I get that!? I'm unsure of the same! @ale-rt

@rohnsha0 I think @ale-rt was suggesting to replace the string parent with ancestor throughout the code, docs, and news item. Before you do that, let's get his confirmation. "Ancestor" is a more generic term than "parent", and includes parents, grandparents, great-greatparents, and further ancestors up the hierarchy.

@ale-rt
Copy link
Member

ale-rt commented Feb 21, 2025

@rohnsha0 I think @ale-rt was suggesting to replace the string parent with ancestor throughout the code, docs, and news item. Before you do that, let's get his confirmation. "Ancestor" is a more generic term than "parent", and includes parents, grandparents, great-greatparents, and further ancestors up the hierarchy.

Thanks @stevepiercy 🥇

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.

Add api.content.get_parents
5 participants