Skip to content

Comments

ci: add hint validation checks#898

Merged
cjdcordeiro merged 15 commits intocanonical:mainfrom
cjdcordeiro:validate-hints
Feb 23, 2026
Merged

ci: add hint validation checks#898
cjdcordeiro merged 15 commits intocanonical:mainfrom
cjdcordeiro:validate-hints

Conversation

@cjdcordeiro
Copy link
Collaborator

@cjdcordeiro cjdcordeiro commented Feb 10, 2026

Proposed changes

Following canonical/chisel#263, this PR introduces:

Related issues/PRs

canonical/chisel#263

Checklist

Additional Context

Example of a test-ci run: https://github.com/cjdcordeiro/chisel-releases/actions/runs/21833297147
Example of a PR with validation of hints: https://github.com/cjdcordeiro/chisel-releases/actions/runs/21853592833/job/63065485307?pr=35

@cjdcordeiro cjdcordeiro requested a review from a team February 10, 2026 06:27
@lczyk lczyk self-assigned this Feb 10, 2026
@lczyk lczyk self-requested a review February 10, 2026 11:51
@ROCKsBot ROCKsBot requested a review from a team February 11, 2026 01:41
Copy link

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

This is very nice, thanks!

@cjdcordeiro cjdcordeiro requested a review from upils February 12, 2026 14:08
@cjdcordeiro
Copy link
Collaborator Author

Copy link

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

Thanks!

Copy link
Collaborator

@lczyk lczyk left a comment

Choose a reason for hiding this comment

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

'request changes' but only due to volume of nitpicks / minor changes requested. overall happy with :)) ty!

@lczyk
Copy link
Collaborator

lczyk commented Feb 16, 2026

also

def not_too_long(text: str, max_length: int = 40) -> ErrorMessage | None:
    """Check that the text is not too long."""
    if len(text) > max_length:
        return f"must be at most {max_length} characters long (found {len(text)})"
    return None


def only_single_spaces(text: str) -> ErrorMessage | None:
    """Check that the text does not contain multiple consecutive spaces."""
    if "  " in text:
        return "multiple consecutive spaces are not allowed"
    return None


def no_leading_trailing_spaces(text: str) -> ErrorMessage | None:
    """Check that the text does not have leading or trailing spaces."""
    if text != text.strip():
        return "leading and trailing spaces are not allowed"
    return None

???

@lczyk
Copy link
Collaborator

lczyk commented Feb 16, 2026

also

    def test_all_caps_acronyms(self, tmp_path):
        f = tmp_path / "sdf.yaml"
        yaml_content = """slices:
  with-acronym:
    hint: Configure the CPU and GPU settings
  
  starting-with-acronym:
    hint: HTTP and HTTPS protocols
"""
        f.write_text(yaml_content, encoding="utf-8")
        errors = validate_hints.validate_hints(str(f))
        assert len(errors) == 0

this is currently passing, which is great, but id add it as a regression test / placeholder if someone adds something to do with acronyms.. ? 🤔

from https://documentation.ubuntu.com/chisel/en/latest/reference/chisel-releases/slice-definitions/:

Uppercase for acronyms (e.g. HTTP not Http).

@cjdcordeiro
Copy link
Collaborator Author

also

def not_too_long(text: str, max_length: int = 40) -> ErrorMessage | None:
    """Check that the text is not too long."""
    if len(text) > max_length:
        return f"must be at most {max_length} characters long (found {len(text)})"
    return None


def only_single_spaces(text: str) -> ErrorMessage | None:
    """Check that the text does not contain multiple consecutive spaces."""
    if "  " in text:
        return "multiple consecutive spaces are not allowed"
    return None


def no_leading_trailing_spaces(text: str) -> ErrorMessage | None:
    """Check that the text does not have leading or trailing spaces."""
    if text != text.strip():
        return "leading and trailing spaces are not allowed"
    return None

???

  • Chisel already checks for the length of the hint - see above
  • disallowing consecutive spaces, although not in the policy, is a good sanitization step IMO. See cd34fce
  • Trailing spaces are already disallowed in the existing validators

also

    def test_all_caps_acronyms(self, tmp_path):
        f = tmp_path / "sdf.yaml"
        yaml_content = """slices:
  with-acronym:
    hint: Configure the CPU and GPU settings
  
  starting-with-acronym:
    hint: HTTP and HTTPS protocols
"""
        f.write_text(yaml_content, encoding="utf-8")
        errors = validate_hints.validate_hints(str(f))
        assert len(errors) == 0

this is currently passing, which is great, but id add it as a regression test / placeholder if someone adds something to do with acronyms.. ? 🤔

from https://documentation.ubuntu.com/chisel/en/latest/reference/chisel-releases/slice-definitions/:

Uppercase for acronyms (e.g. HTTP not Http).

This is a tricky one. It's the same for

Semicolons to separate multiple unrelated fragments of information.

Both require context, and although the NLP might be able to do a good job at it, it won't do it with a high level of confidence. I guess we gotta live with manual enforcement of these for now

@cjdcordeiro cjdcordeiro requested a review from lczyk February 17, 2026 11:25
Copy link
Collaborator

@lczyk lczyk left a comment

Choose a reason for hiding this comment

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

ty for the changes! :)) still marking as 'request changes` but only pending the resolution of #898 (comment) + 2 nits. no further comments

cjdcordeiro and others added 3 commits February 17, 2026 17:15
@cjdcordeiro cjdcordeiro requested a review from lczyk February 17, 2026 17:04
Copy link
Collaborator

@lczyk lczyk left a comment

Choose a reason for hiding this comment

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

thanks! big +1 from me. given lots of changes fro my review, @upils could you have a quick look over? the biggest functionality change is the new no_consecutive_spaces validator. other than that it's "backend" changes.

( adding the 'ready to merge' label but not merging yet )

@lczyk lczyk added the ready to merge This PR is ready to be merged label Feb 18, 2026
Copy link

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

LGTM, thanks!

@cjdcordeiro cjdcordeiro merged commit 3af8beb into canonical:main Feb 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants