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 reverseChunked(by:) Method Implementation #465

Merged

Conversation

jiahan-wu
Copy link
Contributor

@jiahan-wu jiahan-wu commented Mar 8, 2024

Summary

This pull request is being made to resolve an inconsistency found in the reverseChunked(by:) method located within the Collection extension. The intended functionality of the method is to divide a collection into chunks of a given size, where, if the collection is not evenly divisible, the first chunk will be smaller. Instead of this expected behavior, the current implementation produces incorrect results, with the first chunk not being the smallest when required and subsequent chunks varying incorrectly in size.

Example

Here are concrete examples that demonstrate the problem with the reverseChunked(by:) method's current implementation:

Example 1:

let collection = "1234567898765"
let chunks = collection.reverseChunked(by: 4)

// Expected Chunks: ["1", "2345", "6789", "8765"]
// Actual Chunks:   ["1", "2345", "67898", "765"]

Example 2:

let collection = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17]
let chunks = collection.reverseChunked(by: 4)

// Expected Chunks: [[1], [2, 3, 4, 5], [6, 7, 8, 9], [10, 11, 12, 13], [14, 15, 16, 17]]
// Actual Chunks:   [[1], [2, 3, 4, 5], [6, 7, 8, 9, 10], [11, 12, 13, 14, 15, 16, 17], []]

Impact of Changes

The changes introduced by this pull request will resolve the crashing issue in the PostgresNumeric(decimalString:) initializer. The crash is currently caused by the improper handling of string to numeric conversion when the string is split into chunks using the reverseChunked(by:) method.

Screenshot 2024-03-08 at 10 10 48 PM

@gwynne gwynne requested a review from fabianfett March 8, 2024 14:53
@gwynne gwynne added bug Something isn't working semver-patch No public API change. labels Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.76%. Comparing base (43929b0) to head (6eb5201).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   61.78%   61.76%   -0.03%     
==========================================
  Files         125      125              
  Lines       10030    10024       -6     
==========================================
- Hits         6197     6191       -6     
  Misses       3833     3833              
Files Coverage Δ
...ources/PostgresNIO/Data/PostgresData+Numeric.swift 70.50% <100.00%> (-0.86%) ⬇️

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.

At some point we should really just replace those methods altogether with an import of Algorithms and use of .chunks(ofCount:).reversed(), but for now this is fine.

@fabianfett
Copy link
Collaborator

@jiahan-wu Thanks for your contribution! Great fix!

@fabianfett fabianfett merged commit 6f0fc05 into vapor:main Mar 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants