Skip to content

Add 'Assert' consistency to algorithm format check#1168

Merged
leebyron merged 4 commits intomainfrom
assert
Jun 5, 2025
Merged

Add 'Assert' consistency to algorithm format check#1168
leebyron merged 4 commits intomainfrom
assert

Conversation

@benjie
Copy link
Copy Markdown
Member

@benjie benjie commented Jun 2, 2025

We have a mixture of - Assert: blah and - Assert blah. We should standardize on one pattern; this adds it to the spec format checks. (It also fixes some minor bugs in the script itself.)

@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 2, 2025 •
edited
Loading

✅ Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 9405021
🔍 Latest deploy log https://app.netlify.com/projects/graphql-spec-draft/deploys/683d3c8a4dae750008c66990
😎 Deploy Preview https://deploy-preview-1168--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Jun 2, 2025
martinbonnin
martinbonnin approved these changes Jun 5, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

Nit: I like it better without the : since we don't have Return: foo but Return foo and more generally I find that the spec are usually closer to natural language but consistency > everything so I'm fine with everything and support enforcing this in the format check 👍

@benjie
Copy link
Copy Markdown
Member Author

benjie commented Jun 5, 2025 •
edited
Loading

Happy to change to the reverse if the group agrees. I favour Assert: because assertions are kind of optional/documentation steps - they should hold already and should never trigger unless there's a bug - so they could theoretically be omitted from the algorithm. The same cannot be said for a "return" statements 😉

They're more like Note: ... to me than Return ....

@leebyron
Copy link
Copy Markdown
Collaborator

leebyron commented Jun 5, 2025

love this, thanks for adding the check

@leebyron leebyron merged commit 3b0d8e6 into main Jun 5, 2025
9 checks passed
@leebyron leebyron deleted the assert branch June 5, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robrichard robrichard robrichard approved these changes

@martinbonnin martinbonnin martinbonnin approved these changes

@PascalSenn PascalSenn PascalSenn approved these changes

+1 more reviewer

@BoD BoD BoD approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

✏️ Editorial PR is non-normative or does not influence implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@benjie @leebyron @BoD @robrichard @martinbonnin @PascalSenn