Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace regex with winnow based parser #255

Merged
merged 8 commits into from
Jan 21, 2025
Merged

Replace regex with winnow based parser #255

merged 8 commits into from
Jan 21, 2025

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jan 7, 2025

Replace the existing Regex based "parser" with an actual honest-to-goodness parser/combinator based parser.

How to review

Some suggestions on how to review this:

  • Proofread/comment on SPEC.md in the fist commit. This is the basis for the rest of the parser work so if you disagree with something there, it's not worth continuing on into the implementation
  • Read the changes to the integration tests on the final result: Warnings have been added, but otherwise assert that it's a refactor.
  • Read the added parsing error message in error.rs
  • Read the unit tests in procfile.rs. Some of them were useful for development, the majority are asserting overall behavior and edge cases
  • If you want to review the actual winnow and parser code, please book a pairing session with me. It's a lot and is better to talk through syncronously. Feel free to suggest any documentation typos etc.

@schneems
Introduce Procfile format specification
8787d43 
This file is intended to define valid `Procfile` inputs and parser behavior separate from implementation. It may be updated in the future based on upstream requirements (CNB spec or kubernetes spec) or through future discussion.
@schneems
Replace regex with full parser
c0f272d
@schneems
Emit warnings from parser in the buildpack
c99966b
@schneems
Format parsing error
2f7fb77
@schneems
Rename error struct
fef432b
@schneems
Fix use statements
f50bdf9
@schneems
Changelog
de443c8
@schneems schneems marked this pull request as ready for review January 7, 2025 18:33
@schneems schneems requested a review from a team as a code owner January 7, 2025 18:33
@schneems schneems enabled auto-merge (squash) January 7, 2025 19:24
joshwlewis
joshwlewis reviewed Jan 7, 2025
View reviewed changes
SPEC.md Outdated Show resolved Hide resolved
joshwlewis
joshwlewis reviewed Jan 7, 2025
View reviewed changes
SPEC.md Show resolved Hide resolved
joshwlewis
joshwlewis reviewed Jan 7, 2025
View reviewed changes
SPEC.md Show resolved Hide resolved
@schneems
[Stacked] Spec updates (#256)
7ef3a97 
* Use "separated" instead of terminated/preceded

* Add a missing word

* Specify that spaces are not part of they key

* Document duplicate key behavior
@schneems schneems requested a review from a team January 9, 2025 21:55
joshwlewis
joshwlewis reviewed Jan 14, 2025
View reviewed changes
SPEC.md Show resolved Hide resolved
joshwlewis
joshwlewis approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@joshwlewis joshwlewis left a comment

Choose a reason for hiding this comment

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

This probably warrants a major version bump. Looks good to me!

@schneems schneems merged commit fabca6d into main Jan 21, 2025
4 checks passed
@schneems schneems deleted the schneems/procfile-spec branch January 21, 2025 19:29
heroku-linguist bot added a commit that referenced this pull request Jan 21, 2025
@heroku-linguist
Prepare release v4.0.0
91e48fd 
## heroku/procfile

### Changed

- Regex based parser has been removed in favor of a `winnow` based parser combinator, the format is now more strict. See [SPEC.md](spec.md) for more details. ([#255](#255))
    - Keys starting with spaces now emit a warning
    - Underscore key characters (`_`) are now converted to hyphens (`-`) and emit a warning
    - Uppercase key characters are now converted to lowercase and emit a warning
    - Invalid keys now error, previously they were ignored
@heroku-linguist heroku-linguist bot mentioned this pull request Jan 21, 2025
Merged
heroku-linguist bot added a commit that referenced this pull request Jan 21, 2025
@heroku-linguist
Prepare release v4.0.0 (#259)
c441e81 
## heroku/procfile

### Changed

- Regex based parser has been removed in favor of a `winnow` based parser combinator, the format is now more strict. See [SPEC.md](spec.md) for more details. ([#255](#255))
    - Keys starting with spaces now emit a warning
    - Underscore key characters (`_`) are now converted to hyphens (`-`) and emit a warning
    - Uppercase key characters are now converted to lowercase and emit a warning
    - Invalid keys now error, previously they were ignored

Co-authored-by: heroku-linguist[bot] <136119646+heroku-linguist[bot]@users.noreply.github.com>
heroku-linguist bot added a commit to heroku/cnb-builder-images that referenced this pull request Jan 21, 2025
@heroku-linguist
Update heroku/buildpacks-procfile to v4.0.0
9508604 
## heroku/procfile

### Changed

- Regex based parser has been removed in favor of a `winnow` based parser combinator, the format is now more strict. See [SPEC.md](spec.md) for more details. ([#255](heroku/buildpacks-procfile#255))
    - Keys starting with spaces now emit a warning
    - Underscore key characters (`_`) are now converted to hyphens (`-`) and emit a warning
    - Uppercase key characters are now converted to lowercase and emit a warning
    - Invalid keys now error, previously they were ignored
@heroku-linguist heroku-linguist bot mentioned this pull request Jan 21, 2025
Merged
heroku-linguist bot added a commit to heroku/cnb-builder-images that referenced this pull request Jan 21, 2025
@heroku-linguist
Update heroku/buildpacks-procfile to v4.0.0 (#639)
fe9aa63 
## heroku/procfile

### Changed

- Regex based parser has been removed in favor of a `winnow` based parser combinator, the format is now more strict. See [SPEC.md](spec.md) for more details. ([#255](heroku/buildpacks-procfile#255))
    - Keys starting with spaces now emit a warning
    - Underscore key characters (`_`) are now converted to hyphens (`-`) and emit a warning
    - Uppercase key characters are now converted to lowercase and emit a warning
    - Invalid keys now error, previously they were ignored

Co-authored-by: heroku-linguist[bot] <136119646+heroku-linguist[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Malax Malax Malax left review comments

@edmorley edmorley edmorley left review comments

@joshwlewis joshwlewis joshwlewis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@schneems @Malax @edmorley @joshwlewis