Skip to content

feat: add 'for' statement #108

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

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

feat: add 'for' statement #108

wants to merge 2 commits into from

Conversation

mr-cal
Copy link
Contributor

@mr-cal mr-cal commented Aug 6, 2025

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint && make test?

Adds a for statement to craft-grammar. This is the first step towards a new variant of advanced grammar, so some restrictions apply:

  • 'for' can't be combined with other statements.
  • only one selector may be used in a 'for' statement (for a,b is invalid)

(IMAGECRAFT-38)

@mr-cal mr-cal requested a review from Copilot August 6, 2025 20:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new 'for' statement to craft-grammar as part of implementing a new variant of advanced grammar. The 'for' statement allows matching against platforms and includes restrictions to prevent mixing with other statement types.

  • Adds ForStatement class with platform matching functionality
  • Implements variant checking to prevent mixing 'for' with other grammar statements
  • Updates base processor to support platforms parameter

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
craft_grammar/_for.py New ForStatement implementation with platform matching logic
craft_grammar/_processor.py Adds variant tracking and 'for' statement parsing
craft_grammar/_base_processor.py Extends base to accept platforms parameter
craft_grammar/errors.py Adds ForStatementSyntaxError exception class
tests/unit/test_for.py Comprehensive test coverage for ForStatement
tests/unit/test_processor.py Tests for variant mixing prevention
tests/unit/test_to.py Corrects test function names from 'on' to 'to'
tests/unit/test_errors.py Tests for new ForStatementSyntaxError
pyproject.toml Adds "Afor" to codespell ignore list
craft_grammar/init.py Exports ForStatement class

@mr-cal mr-cal force-pushed the work/IMAGECRAFT-38-for branch from 065ff50 to 5e716bf Compare August 6, 2025 20:35
Signed-off-by: Callahan Kovacs <[email protected]>
@mr-cal mr-cal force-pushed the work/IMAGECRAFT-38-for branch from 5e716bf to 0760b13 Compare August 7, 2025 12:24
@mr-cal mr-cal marked this pull request as ready for review August 7, 2025 12:46
@mr-cal mr-cal requested review from tigarmo, lengau and upils August 7, 2025 12:47
Copy link
Contributor

@upils upils left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I was expecting a bigger change initially, but this is clean and nice!


New features:

- Add a new 'for' statement to select against a platform.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Add a new 'for' statement to select against a platform.
- Add a new ``for`` statement to select against a platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 493f9fc


.. _release 2.1.0:

2.1.0 (2025-08-06)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2.1.0 (2025-08-06)
2.1.0 (2025-08-11)

Even if we merge today let's do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, this should have been undated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 493f9fc

@@ -53,12 +70,15 @@ def __init__(
returning true if it is valid.
:param arch: the architecture the system is on.
:param target_arch: the architecture the system is to build for.
:param platforms: the platforms to build. Duplicates are ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since for a,b is invalid, what is the use-case for platforms having more than 1 item? Does this refer to all the possible platforms that a project declares, or just to the single platform that is currently being built?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recall this discussion during the Frankfurt sprint but I don't know if it was written down:

Given

When craft-application goes to build the [email protected]:riscv64 platform, craft-grammar could be invoked with

GrammarProcessor(
  arch="riscv64",
  target_arch="riscv64",
  platforms=["[email protected]:riscv64", "24.04", "ubuntu", "riscv64"],
)

By default, craft-application would pass a single element list containing the entire platform name. An app can choose to override this and generate a list, like in the example above.

I'm fine to change this parameter to a string or clarify the docstrings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see, I remember some of that conversation now. I think I tripped on "the platforms to build", it made me think of the full set of platforms. Maybe something like "the currently active platform identifiers" would be less confusing? Well, less confusing in this particular aspect I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I cleaned up the whole docstring: 493f9fc

Signed-off-by: Callahan Kovacs <[email protected]>
@mr-cal mr-cal requested review from upils and lengau August 8, 2025 20:46
@mr-cal mr-cal added the PR: Squash This PR should be a squash commit. label Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Squash This PR should be a squash commit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants