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.

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

Fix crash with missing parameter #130

Merged
merged 5 commits into from
Mar 20, 2024

Conversation

b-nassler
Copy link
Contributor

@b-nassler b-nassler commented Mar 4, 2024

These changes are now available in 1.10.5

This line https://github.com/vapor/leaf-kit/blob/main/Sources/LeafKit/LeafParser/LeafParser.swift#L203
would cause a crash when no parameters are found

Fixed issue that lead to a crash when parameters would be empty in line https://github.com/vapor/leaf-kit/blob/main/Sources/LeafKit/LeafParser/LeafParser.swift#L203

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Nice catch and a great fix! Just a couple of nitpicks, see the inline comments 👍

Sources/LeafKit/LeafParser/LeafParser.swift Outdated Show resolved Hide resolved
Sources/LeafKit/LeafParser/LeafParser.swift Outdated Show resolved Hide resolved
Sources/LeafKit/LeafError.swift Outdated Show resolved Hide resolved
Sources/LeafKit/LeafParser/LeafParser.swift Show resolved Hide resolved
@gwynne
Copy link
Member

gwynne commented Mar 4, 2024

Oh, I forgot to also add - can you add a test for this as well?

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 82.09%. Comparing base (38e3144) to head (750db9d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
- Coverage   82.15%   82.09%   -0.07%     
==========================================
  Files          25       25              
  Lines        2595     2603       +8     
==========================================
+ Hits         2132     2137       +5     
- Misses        463      466       +3     
Files Coverage Δ
Sources/LeafKit/LeafParser/LeafParser.swift 94.05% <83.33%> (-0.91%) ⬇️

@b-nassler b-nassler requested a review from gwynne March 5, 2024 20:56
@b-nassler
Copy link
Contributor Author

@gwynne Updated, please check 🙂

@b-nassler
Copy link
Contributor Author

@gwynne @0xTim Everything should be ok I think. Let me know if I should do anything else. Thanks!

gwynne
gwynne previously requested changes Mar 8, 2024
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Great! One last nit and all should be good to go 🙂

Tests/LeafKitTests/LeafErrorTests.swift Outdated Show resolved Hide resolved
@b-nassler
Copy link
Contributor Author

@gwynne Good one! I committed your suggestion

Sources/LeafKit/LeafError.swift Outdated Show resolved Hide resolved
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thank you!

@0xTim 0xTim added the semver-patch Internal changes only label Mar 14, 2024
@0xTim 0xTim enabled auto-merge (squash) March 14, 2024 10:21
@b-nassler
Copy link
Contributor Author

@0xTim You probably saw this but something ran out of memory so it didn't succeed merging

@0xTim 0xTim changed the title fixed missing parameter causing crash Fix crash with missing parameter Mar 20, 2024
@0xTim 0xTim merged commit f6a08a1 into vapor:main Mar 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants