Issue #533: Implement API get_vocabulary & get_vocabularies_name #557

ujsquared · 2025-02-11T17:32:37Z

Fixes Issue #533

Uses implementation from kitconcept.api, as mentioned in the issue description. Includes both the APIs present in source.
Tests written and documentation done.

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

…get-vocabulary

mister-roboto · 2025-02-11T17:32:45Z

@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:

@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!

ujsquared · 2025-02-11T17:58:15Z

@jenkins-plone-org please run jobs

stevepiercy · 2025-02-11T22:53:58Z

@ujsquared please edit the pull request description as described in Item 2, second bullet point, in https://6.docs.plone.org/contributing/first-time.html#create-a-pull-request-from-your-fork.

stevepiercy

Docs look good to me. Minor changes requested to the change log. Thank you!

Needs technical review. Also where and when do we run and verify the doctests, that is, the lines of code with a leading % in the docs?

Direct links to pull request preview:

news/533.feature

stevepiercy · 2025-02-11T23:29:36Z

Strange, we say to include doctests in https://6.docs.plone.org/plone.api/contribute.html#add-a-function-to-an-existing-module, but don't say how to actually run them or verify them in CI. What did I miss?

As a quick hack, I added this to tox.ini on line 302 as a quick hack:

    sphinx-build -b doctest -d _build/docs/doctrees docs _build/docs/doctest

And running tox -e docs results in no doctests found.

Doctest summary
===============
    0 tests
    0 failures in tests
    0 failures in setup code
    0 failures in cleanup code

@ksuess do you have any idea? I recall you worked on this 2 or 3 years ago.

ujsquared · 2025-02-12T06:35:04Z

Needs technical review. Also where and when do we run and verify the doctests, that is, the lines of code with a leading % in the docs?

I have no idea too, /src/plone/api/tests/doctests/ only contains symlinks to docs/.

ale-rt · 2025-02-12T07:32:03Z

src/plone/api/content.py

+    try:
+        vocab = getUtility(IVocabularyFactory, name)
+    except ComponentLookupError:
+        raise InvalidParameterError(f"No vocabulary with name '{name}' available.")


Thanks a lot for your contribution!
It might be interesting to have an error message consistent with the one raised by the get_view function:

plone.api/src/plone/api/content.py

Lines 534 to 554 in c9a35d7

except ComponentLookupError:

# Getting all available views

sm = getSiteManager()

available_views = sm.adapters.lookupAll(

required=(providedBy(context), providedBy(request)),

provided=Interface,

)

# Check if the requested view is available

# by getting the names of all available views

available_view_names = [view[0] for view in available_views]

if name not in available_view_names:

# Raise an error if the requested view is not available.

raise InvalidParameterError(

"Cannot find a view with name '{name}'.\n"

"Available views are:\n"

"{views}".format(

name=name,

views="\n".join(sorted(available_view_names)),

),

)

What do you think?

Agreed. Making the changes.

ale-rt · 2025-02-12T07:36:45Z

src/plone/api/content.py

+    :return: A list of vocabularies names.
+    """
+    all_vocabs = getUtilitiesFor(IVocabularyFactory)
+    return [v[0] for v in all_vocabs]


What do you think about:

Suggested change

return [v[0] for v in all_vocabs]

return sorted([name for name, vocabulary in getUtilitiesFor(IVocabularyFactory)])

?

ale-rt · 2025-02-12T07:38:14Z

src/plone/api/tests/test_content.py

+        self.assertTrue(vocab)
+        self.assertTrue(hasattr(vocab, "__iter__"))  # Verify it's iterable


I would replace these lines with a self.assertIsInstance

would you suggest importing collections.abc just to replace current approach?
i can try going with this

but i'm not sure if vocabularies with length 0 are to be supported or not.

Nope, sorry. I was referring to something like:

Suggested change

self.assertTrue(vocab)

self.assertTrue(hasattr(vocab, "__iter__")) # Verify it's iterable

self.assertIsInstance(vocabulary, SimpleVocabulary)

ale-rt

Great work!
Thanks for your contribution.
I added some suggestion.

I would not be cheap with variable names.
I would replace vocab with vocabulary and vocabs with vocabularies.

ksuess · 2025-02-12T08:58:42Z

Strange, we say to include doctests in https://6.docs.plone.org/plone.api/contribute.html#add-a-function-to-an-existing-module, but don't say how to actually run them or verify them in CI. What did I miss?

As a quick hack, I added this to tox.ini on line 302 as a quick hack:
    sphinx-build -b doctest -d _build/docs/doctrees docs _build/docs/doctest
And running tox -e docs results in no doctests found.
Doctest summary
===============
    0 tests
    0 failures in tests
    0 failures in setup code
    0 failures in cleanup code
@ksuess do you have any idea? I recall you worked on this 2 or 3 years ago.

Well, yes, the "contributing" chapter lacks the mentioning of tox -e test.
tox -e test runs all tests, including the doctests.
Will raise a PR.

ksuess

Thanks a lot for your contribution! Awesome.

I think, as most of the vocabularies are not context sensitive, these two new methods would fit better in api.portal than in api.content

ksuess · 2025-02-12T09:13:14Z

src/plone/api/tests/test_content.py

+        self.assertIn("private", states)
+        self.assertIn("published", states)
+
+    def test_vocabulary_context_sensitivity(self):


This test on context sensitivity would need a vocabulary that is context sensitive to be meaningful.
I would say this test is not really needed, as the new plone.api method api.content.get_vocabulary is simply calling getUtility(IVocabularyFactory, name) and applying this vocabulary to the context. So the underlying code is testing the context sensitivity already.
Further opinions are welcome.

src/plone/api/content.py

ksuess · 2025-02-12T18:01:20Z

src/plone/api/tests/test_portal.py

+        vocabulary = portal.get_vocabulary(name="plone.app.vocabularies.PortalTypes")
+        self.assertIsInstance(
+            vocabulary, SimpleVocabulary
+        )  # Check for correct interface


this tests on being vocabulary an instance of a class. The comment is not correct. Better no comment.

Earlier, I was testing for IVocabularyTokenized interface. Missed updating the comment

src/plone/api/tests/test_portal.py

docs/portal.md

ujsquared · 2025-02-13T08:12:52Z

@jenkins-plone-org please run jobs

src/plone/api/portal.py

ksuess

Looks good to me. Well done, @ujsquared.

The failing test is unrelated.
I approve.

ale-rt · 2025-02-13T09:29:51Z

I think, as most of the vocabularies are not context sensitive, these two new methods would fit better in api.portal than in api.content

@ksuess I am not really sure about that.
To me, it seems that api.content is a better fit, but I do not have a strong opinion.

Maybe some other people might share their feelings about where these helpers should go.

CC @plone/framework-team

ale-rt

This looks fine to me.
Please wait the other reviewers approval before merging.

src/plone/api/portal.py

@ale-rt

as suggested by @ale-rt Co-authored-by: Alessandro Pisa <[email protected]>

ale-rt · 2025-02-13T13:37:22Z

@jenkins-plone-org please run jobs

ale-rt · 2025-02-14T10:17:11Z

@stevepiercy I am merging this, I hope you are fine with that.

stevepiercy · 2025-02-14T10:24:02Z

@ale-rt please wait for me to complete my review. There is no rush.

stevepiercy · 2025-02-17T11:53:45Z

I'd like to hold off on merging this PR until @ksuess's PR #558 is merged, as that one affects this PR and it provides welcome improvements.

I'll submit a review after that PR is merged to verify that make test works as expected.

stevepiercy · 2025-02-17T13:03:04Z

Well, yes, the "contributing" chapter lacks the mentioning of tox -e test.
tox -e test runs all tests, including the doctests.
Will raise a PR.

@ksuess I don't see how tox -e test runs doctests, which is invoked with sphinx-build doctests. What am I missing?

ksuess · 2025-02-17T15:37:55Z

tox -e test runs all tests, including the doctests.

@ksuess I don't see how tox -e test runs doctests, which is invoked with sphinx-build doctests. What am I missing?

Tests in doctests/*.md, both code snippets and invisible code blocks, are tested in https://github.com/plone/plone.api/blob/main/src/plone/api/tests/test_doctests.py. It uses manuel TestSuite.

stevepiercy · 2025-02-17T18:28:55Z

Tests in doctests/*.md, both code snippets and invisible code blocks, are tested in https://github.com/plone/plone.api/blob/main/src/plone/api/tests/test_doctests.py. It uses manuel TestSuite.

To verify, I altered one of the doctests in contribute.md so that its assertion would fail. make test passed. Maybe I misunderstand what is the purpose of % invisible-code-block: python?

However, when I change a doctest in user.md:

% self.assertEqual(user.getProperty('email'), '[email protected]')

make test fails.

So maybe the test itself does not get called? Ah, there is a missing symlink for contribute.md! Should we test contribute.md as well? I mean, there are doctests in it...

ksuess · 2025-02-17T18:38:27Z

Should we test contribute.md as well? I mean, there are doctests in it...

Yes, it would make sense to test the example tests. Would you be so kind to add a symlink in tests/doctests? Thank you!

davisagli · 2025-02-18T03:21:40Z

Yes, it would make sense to test the example tests. Would you be so kind to add a symlink in tests/doctests?

@stevepiercy @ksuess Done in 3d1ae92

…ssue-533-add-get-vocabulary

…_name`

…squared/plone.api into issue-533-add-get-vocabulary

- Align label and target with heading - Use `{func}` for function

stevepiercy

This looks great! Let's merge when green.

stevepiercy · 2025-02-18T10:54:49Z

@jenkins-plone-org please run jobs

docs/portal.md

Co-authored-by: Alessandro Pisa <[email protected]>

stevepiercy · 2025-02-18T11:20:59Z

@jenkins-plone-org please run jobs

stevepiercy · 2025-02-18T11:40:18Z

Looks like Jenkins does not like some tests. What should we do?

ujsquared · 2025-02-18T13:27:33Z

Looks like Jenkins does not like some tests. What should we do?

Out of curiosity, isn't this the one that failed earlier too? Even locally I'm running into issues when testing on python3.9. Any fixes?

stevepiercy · 2025-02-18T13:32:17Z

And now they're all green. ¯\_(ツ)_/¯ I'm gonna merge.

ujsquared added 2 commits February 11, 2025 22:40

Issue 533: Add api.content.get_vocabulary

ec67622

Merge branch 'main' of github.com:plone/plone.api into issue-533-add-…

b24a7fc

…get-vocabulary

update change log

df5429c

stevepiercy requested changes Feb 11, 2025

View reviewed changes

news/533.feature Outdated Show resolved Hide resolved

news/533.feature Outdated Show resolved Hide resolved

news/533.feature Outdated Show resolved Hide resolved

news/533.feature Outdated Show resolved Hide resolved

update: changelog fixes

15441eb

ale-rt reviewed Feb 12, 2025

View reviewed changes

ksuess reviewed Feb 12, 2025

View reviewed changes

ujsquared added 2 commits February 12, 2025 21:37

fix: make review changes

1ade6d8

fix: lint issues

0a4e42d

ksuess requested changes Feb 12, 2025

View reviewed changes

ksuess linked an issue Feb 12, 2025 that may be closed by this pull request

Add api.content.get_vocabulary #533

Closed

ujsquared added 2 commits February 13, 2025 02:33

fix: suggested changes

ccfe146

fix: linting issues

3392922

ale-rt reviewed Feb 13, 2025

View reviewed changes

src/plone/api/portal.py Outdated Show resolved Hide resolved

ksuess approved these changes Feb 13, 2025

View reviewed changes

ksuess requested review from stevepiercy and ale-rt February 13, 2025 09:04

ale-rt approved these changes Feb 13, 2025

View reviewed changes

ale-rt reviewed Feb 13, 2025

View reviewed changes

src/plone/api/portal.py Outdated Show resolved Hide resolved

fix: follow PEP8 guidelines for none check

3078ab3

as suggested by @ale-rt Co-authored-by: Alessandro Pisa <[email protected]>

ujsquared and others added 5 commits February 18, 2025 13:38

Merge branch 'main' of github.com-personal:ujsquared/plone.api into i…

98b5024

…ssue-533-add-get-vocabulary

fix: make suggested changes `get_vocabularies_names => get_vocabulary…

caf9226

…_name`

Merge branch 'issue-533-add-get-vocabulary' of github.com-personal:uj…

f07420b

…squared/plone.api into issue-533-add-get-vocabulary

Minor touch-up

d806295

- Align label and target with heading - Use `{func}` for function

Use reStructuredText markup for CHANGES.rst

91bd500

stevepiercy approved these changes Feb 18, 2025

View reviewed changes

ale-rt reviewed Feb 18, 2025

View reviewed changes

docs/portal.md Outdated Show resolved Hide resolved

Update docs/portal.md

4685693

Co-authored-by: Alessandro Pisa <[email protected]>

ale-rt approved these changes Feb 18, 2025

View reviewed changes

stevepiercy merged commit a876d70 into plone:main Feb 18, 2025
16 checks passed

	except ComponentLookupError:
	# Getting all available views
	sm = getSiteManager()
	available_views = sm.adapters.lookupAll(
	required=(providedBy(context), providedBy(request)),
	provided=Interface,
	)

	# Check if the requested view is available
	# by getting the names of all available views
	available_view_names = [view[0] for view in available_views]
	if name not in available_view_names:
	# Raise an error if the requested view is not available.
	raise InvalidParameterError(
	"Cannot find a view with name '{name}'.\n"
	"Available views are:\n"
	"{views}".format(
	name=name,
	views="\n".join(sorted(available_view_names)),
	),
	)

	return [v[0] for v in all_vocabs]
	return sorted([name for name, vocabulary in getUtilitiesFor(IVocabularyFactory)])

		self.assertTrue(vocab)
		self.assertTrue(hasattr(vocab, "__iter__")) # Verify it's iterable

	self.assertTrue(vocab)
	self.assertTrue(hasattr(vocab, "__iter__")) # Verify it's iterable
	self.assertIsInstance(vocabulary, SimpleVocabulary)

Issue #533: Implement API get_vocabulary & get_vocabularies_name #557

Issue #533: Implement API get_vocabulary & get_vocabularies_name #557

Conversation

ujsquared commented Feb 11, 2025 • edited Loading

mister-roboto commented Feb 11, 2025

ujsquared commented Feb 11, 2025

stevepiercy commented Feb 11, 2025

stevepiercy left a comment

Choose a reason for hiding this comment

stevepiercy commented Feb 11, 2025

ujsquared commented Feb 12, 2025

ale-rt Feb 12, 2025

Choose a reason for hiding this comment

ujsquared Feb 12, 2025

Choose a reason for hiding this comment

ale-rt Feb 12, 2025

Choose a reason for hiding this comment

ale-rt Feb 12, 2025

Choose a reason for hiding this comment

ujsquared Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

ale-rt Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

ale-rt left a comment

Choose a reason for hiding this comment

ksuess commented Feb 12, 2025

ksuess left a comment • edited Loading

Choose a reason for hiding this comment

ksuess Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

ksuess Feb 12, 2025

Choose a reason for hiding this comment

ujsquared Feb 12, 2025

Choose a reason for hiding this comment

ujsquared commented Feb 13, 2025

ksuess left a comment

Choose a reason for hiding this comment

ale-rt commented Feb 13, 2025 • edited Loading

ale-rt left a comment

Choose a reason for hiding this comment

ale-rt commented Feb 13, 2025

ale-rt commented Feb 14, 2025

stevepiercy commented Feb 14, 2025

stevepiercy commented Feb 17, 2025

stevepiercy commented Feb 17, 2025

ksuess commented Feb 17, 2025

stevepiercy commented Feb 17, 2025

ksuess commented Feb 17, 2025

davisagli commented Feb 18, 2025

stevepiercy left a comment

Choose a reason for hiding this comment

stevepiercy commented Feb 18, 2025

stevepiercy commented Feb 18, 2025

stevepiercy commented Feb 18, 2025

ujsquared commented Feb 18, 2025

stevepiercy commented Feb 18, 2025

ujsquared commented Feb 11, 2025 •

edited

Loading

ujsquared Feb 12, 2025 •

edited

Loading

ale-rt Feb 12, 2025 •

edited

Loading

ksuess left a comment •

edited

Loading

ksuess Feb 12, 2025 •

edited

Loading

ale-rt commented Feb 13, 2025 •

edited

Loading