DO NOT MERGE: Review PR for ProjectService and Pytest plugin #672

lengau · 2025-03-04T19:10:34Z

This PR is for reviewing documentation on the hybrid-commands feature branch. Fixes should be collected into another PR.

Stuff to review here:

conf.py
Changelog (source)
pytest plugin (rST source, python source)
ProjectService (rST source, python source)

There will be more later, but this is what's currently on that feature branch.

CRAFT-4305

This adds a pytest plugin for craft-application, including the fixtures: 1. debug_mode: This fixture puts the app into debug mode by default. 2. production_mode: Overrides debug_mode for a function. This is a breaking change because debug_mode is an auto-used fixture, meaning any tests that rely on production mode will break.

…655) This modifies the fake_host_architecture fixture in the pytest plugin so it makes craft_parts also think it's running on that machine.

Removes the ability to use secrets. This can be added to applications if necessary using the prototype secrets service.

This makes the ServiceFactory no longer a dataclass. It's only a dataclass for historical reasons around the old way of registering services, but this is no longer necessary.

This moves set_kwargs() from pending deprecation to deprecated. It doesn't change any behaviour other than the type of warning that it raises.

This adds the project service and makes the relevant changes to the rest of the application to handle it. Some tests have been disabled because of necessary future changes before we can merge this feature branch.

This adds a BuildPlanService that creates build plans for crafts.

These were deprecated, so we can remove them.

This creates partitioncraft, a very basic craft app that uses partitions.

…ft (#674) This modifies the ProjectService so that it will work for Imagecraft. Most specifically, it adds a get_partitions_for method that is a generic partition generator and changes the prime project's partitions to a property. Co-authored-by: Upils <[email protected]>

medubelko

Leaving this review as a comment, since more pages are planned.

I don't feel qualified to review the docstrings, so I left them as-is.

medubelko · 2025-03-08T01:57:38Z

docs/reference/changelog.rst

+- A new :doc:`services/project` now handles the creation and management of the project
+  for this run of the application.


this run

Looking at this sentence in isolation, I'm not quite sure what this means. I might be missing something obvious?

medubelko · 2025-03-08T02:02:27Z

docs/reference/changelog.rst

+- Setting the arguments for a service using the service factory's ``set_kwargs`` is
+  deprecated. Use ``update_kwargs`` instead or file `an issue
+  <https://github.com/canonical/craft-application/issues/new?template=bug.yaml>`_
+  if you still need this method.


We might want to be clearer about our intent here. If this is an experimental change that is subject to change from feedback, I think we should state that plainly.

medubelko · 2025-03-08T02:03:31Z

docs/reference/changelog.rst

+Testing
+=======
+
+- A new :doc:`pytest-plugin` with a fixture that enables production mode for the


Suggested change

- A new :doc:`pytest-plugin` with a fixture that enables production mode for the

- Added a new :doc:`pytest-plugin` with a fixture that enables production mode for the

medubelko · 2025-03-08T02:06:13Z

docs/reference/changelog.rst

Noting here that we're using past tense in this changelog, unlike the product changelogs.

medubelko · 2025-03-08T02:13:31Z

docs/reference/changelog.rst

+Services
+========
+
+- A new :doc:`services/project` now handles the creation and management of the project


I'm a bit iffy on the way we're stating this, since (I believe) we're replacing a class with a service with the same name. Will that have an effect on how it's instantiated?

medubelko · 2025-03-08T02:53:17Z

docs/reference/services/project.rst

+~~~~~~~~~~~~~~~~
+
+At this step, any user-added grammar is validated using
+:external+craft-grammar:doc:`Craft Grammar <index>`. No grammar is applied yet.


applied

Later we use "processed". Same idea?

medubelko · 2025-03-08T02:54:03Z

docs/reference/services/project.rst

+during the application-specific transforms step, meaning that transforms may add
+grammar-aware syntax if needed.
+
+Validate Pydantic model


Suggested change

Validate Pydantic model

Validate the Pydantic model

medubelko · 2025-03-08T02:57:09Z

docs/reference/pytest-plugin.rst

+craft-application includes a `pytest`_ plugin to help ease the testing of apps that use
+it as a framework.


Suggested change

craft-application includes a `pytest`_ plugin to help ease the testing of apps that use

it as a framework.

The pytest plugin makes testing apps that use `pytest`_ easier.

medubelko · 2025-03-08T02:57:43Z

docs/reference/pytest-plugin.rst

+Auto-used fixtures
+~~~~~~~~~~~~~~~~~~


I'd make this an H2.

medubelko · 2025-03-08T02:59:37Z

docs/reference/pytest-plugin.rst

+~~~~~~~~~~~~~~~~~~
+
+Some fixtures are automatically enabled for tests, changing the default behaviour of
+applications during the testing process. This is kept to a minimum, but is done when


Suggested change

applications during the testing process. This is kept to a minimum, but is done when

applications during the testing process. Fixtures generally only change behaviour when

lengau added 7 commits February 20, 2025 11:32

feat!(pytest-plugin): make fake_host_architecture modify craft-parts (#…

0b3b1ed

…655) This modifies the fake_host_architecture fixture in the pytest plugin so it makes craft_parts also think it's running on that machine.

chore: merge branch 'main' into feature/hybrid-commands

f05860f

feat!(secrets): remove secrets (#657)

ee8e89e

Removes the ability to use secrets. This can be added to applications if necessary using the prototype secrets service.

refactor(ServiceFactory): de-dataclass the ServiceFactory (#654)

fbbecee

This makes the ServiceFactory no longer a dataclass. It's only a dataclass for historical reasons around the old way of registering services, but this is no longer necessary.

feat: mark ServiceFactory.set_kwargs() as deprecated (#660)

52b1768

This moves set_kwargs() from pending deprecation to deprecated. It doesn't change any behaviour other than the type of warning that it raises.

feat!(services): add a project service (#646)

83ca560

This adds the project service and makes the relevant changes to the rest of the application to handle it. Some tests have been disabled because of necessary future changes before we can merge this feature branch.

lengau requested a review from medubelko March 4, 2025 19:23

lengau changed the title ~~DO NOT MERGE: Documentation for hybrid-commands feature branch~~ DO NOT MERGE: Review PR for ProjectService and Pytest plugin Mar 4, 2025

lengau requested a review from jahn-junior March 4, 2025 19:23

lengau and others added 4 commits March 4, 2025 16:32

feat!(services): build plan service (#666)

8067351

This adds a BuildPlanService that creates build plans for crafts.

chore!(models): remove BuildInfo and BuildPlanner (#667)

8eddd42

These were deprecated, so we can remove them.

feat(test): create partitioncraft (#673)

729a829

This creates partitioncraft, a very basic craft app that uses partitions.

medubelko reviewed Mar 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DO NOT MERGE: Review PR for ProjectService and Pytest plugin #672

DO NOT MERGE: Review PR for ProjectService and Pytest plugin #672

lengau commented Mar 4, 2025 •

edited

Loading

medubelko left a comment

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

medubelko Mar 8, 2025

		- A new :doc:`services/project` now handles the creation and management of the project
		for this run of the application.

	- A new :doc:`pytest-plugin` with a fixture that enables production mode for the
	- Added a new :doc:`pytest-plugin` with a fixture that enables production mode for the

		craft-application includes a `pytest`_ plugin to help ease the testing of apps that use
		it as a framework.

	craft-application includes a `pytest`_ plugin to help ease the testing of apps that use
	it as a framework.
	The pytest plugin makes testing apps that use `pytest`_ easier.

	applications during the testing process. This is kept to a minimum, but is done when
	applications during the testing process. Fixtures generally only change behaviour when

		Auto-used fixtures
		~~~~~~~~~~~~~~~~~~

DO NOT MERGE: Review PR for ProjectService and Pytest plugin #672

Are you sure you want to change the base?

DO NOT MERGE: Review PR for ProjectService and Pytest plugin #672

Conversation

lengau commented Mar 4, 2025 • edited Loading

medubelko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lengau commented Mar 4, 2025 •

edited

Loading