Skip to content

Sendable fixes and other cleanup #135

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

Merged
merged 17 commits into from
Apr 2, 2025
Merged

Sendable fixes and other cleanup #135

merged 17 commits into from
Apr 2, 2025

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Apr 1, 2025

These changes are now available in 1.13.0

The big changes in this PR are making the code Sendable-clean and minimally strict Concurrency-correct (emphasis on "minimally", there's still a bunch of stuff with futures going on). Accordingly with the release of Swift 6.1, the package also now requires a minimum version of Swift 5.10.

Additional changes:

  • README and docs appearance updated.
  • Enabled Android CI.
  • Replaced the convoluted "performant" HTML escaping logic with straightforward string replacement calls which turn out to actually be more performant than the circa Swift 4.2 unsafe pointer stuff.
  • Fixed the tests so they no longer fail if run on a machine set to a timezone where DST is currently in effect. Tests also no longer use EventLoopFuture.wait().
  • A pass across the board for basic style, formatting, and readability. There's a bit less force-unwrapping going around for good measure.

gwynne added 5 commits April 1, 2025 00:49
…lves a lot of marking things Sendable, or marking things nonisolated(unsafe) in conjunction with a lock. LeafTag.defaultTags remains a distinct problem. Also got rid of the use of NonBlockingFileIO in NIOLeafFiles. Renamed Exports.swift -> Character+Identities.swift.
… of the time than, and at best only equivalent to, just doing .replacing(_:with:) (in 6.0+) or .replacingOccurrences(of:with:) (in 5.10).
…f paths and dates (tests no longer fail for users in DST-active timezones)
@gwynne gwynne added enhancement New feature or request semver-minor Contains new APIs labels Apr 1, 2025
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 72.91213% with 373 lines in your changes missing coverage. Please review.

Project coverage is 80.84%. Comparing base (902c512) to head (8528dea).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rces/LeafKit/LeafSerialize/ParameterResolver.swift 64.31% 76 Missing ⚠️
Sources/LeafKit/LeafData/LeafData.swift 54.54% 50 Missing ⚠️
Sources/LeafKit/LeafSyntax/LeafSyntax.swift 78.10% 44 Missing ⚠️
Sources/LeafKit/LeafParser/LeafParser.swift 79.67% 25 Missing ⚠️
Sources/LeafKit/LeafData/LeafDataStorage.swift 68.91% 23 Missing ⚠️
Sources/LeafKit/LeafParser/LeafParameter.swift 79.09% 23 Missing ⚠️
Sources/LeafKit/LeafConfiguration.swift 41.66% 21 Missing ⚠️
Sources/LeafKit/LeafLexer/LeafParameterTypes.swift 52.27% 21 Missing ⚠️
...urces/LeafKit/LeafData/LeafDataRepresentable.swift 22.72% 17 Missing ⚠️
Sources/LeafKit/LeafSyntax/LeafTag.swift 20.00% 12 Missing ⚠️
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   82.34%   80.84%   -1.51%     
==========================================
  Files          25       25              
  Lines        2617     2678      +61     
==========================================
+ Hits         2155     2165      +10     
- Misses        462      513      +51     
Files with missing lines Coverage Δ
Sources/LeafKit/LeafCache/LeafCache.swift 0.00% <ø> (ø)
Sources/LeafKit/String+HTMLEscape.swift 100.00% <100.00%> (ø)
Sources/LeafKit/LeafLexer/LeafRawTemplate.swift 95.74% <95.83%> (-1.70%) ⬇️
Sources/LeafKit/LeafSerialize/LeafContext.swift 93.75% <75.00%> (ø)
Sources/LeafKit/LeafLexer/LeafToken.swift 90.90% <90.00%> (ø)
...es/LeafKit/LeafSerialize/Dictionary+LeafData.swift 85.71% <66.66%> (-14.29%) ⬇️
Sources/LeafKit/LeafSource/NIOLeafFiles.swift 95.83% <93.54%> (+6.70%) ⬆️
Sources/LeafKit/LeafSerialize/LeafSerializer.swift 94.53% <86.36%> (-0.67%) ⬇️
Sources/LeafKit/Character+Identities.swift 91.11% <91.11%> (ø)
Sources/LeafKit/LeafAST.swift 94.59% <88.88%> (-2.38%) ⬇️
... and 15 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gwynne gwynne mentioned this pull request Apr 1, 2025
…en running anything other than Darwin or a Glibc Linux (i.e. especially not Android)
@gwynne
Copy link
Member Author

gwynne commented Apr 1, 2025

The failure of the integration tests is spurious; the SWIFT_DETERMINISTIC_HASHING requirement is removed by vapor/leaf#237.

Copy link
Member

@ptoffy ptoffy left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, I'd add Vapor's .swift-format file and formatting CI given all the style changes

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.

Couple of minor nits, nothing major

gwynne added 6 commits April 1, 2025 10:40
…ascades from it. It is not used and was cluttering up the code to no purpose. Also cleaned up LeafDataStorage a lot in general and removed a chunk of other unused code on it and LeafData.
…f String to Error, we are not stuck with setting the bad example of actually using that conformance. Remove all such instances, using LeafError(.unknownError()) instead.
…l-only testing support methods we don't need (there is no public interface to directly specify an unsearchable source, not sure why sources even have that feature), exclude all performance tests on Android (they break there), get rid of a few unnecessary force-unwraps, get rid of the last usages of .trimmingCharacters(in:)
@ptoffy ptoffy mentioned this pull request Apr 2, 2025
@gwynne gwynne merged commit 73f4ad4 into main Apr 2, 2025
11 of 13 checks passed
@gwynne gwynne deleted the sendable-and-cleanup branch April 2, 2025 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants