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

Add failing test for regression #184

Closed
wants to merge 1 commit into from

Conversation

stephencelis
Copy link

We have a project that started failing row decoding when upgrading from 3.28.0. I haven't dug into the problem but was able to sketch out a failing test that I believe should pass in 3.28.0 (wasn't able to easily confirm because the testing infrastructure on main didn't exist back then).

We have a project that started failing row decoding when upgrading from 3.28.0. I haven't dug into the problem but was able to sketch out a failing test that I believe should pass in 3.28.0 (wasn't able to easily confirm because the testing infrastructure on `main` didn't exist back then).
@stephencelis stephencelis requested a review from gwynne as a code owner July 3, 2024 19:56
stephencelis added a commit to pointfreeco/pointfreeco that referenced this pull request Jul 3, 2024
stephencelis added a commit to pointfreeco/pointfreeco that referenced this pull request Jul 3, 2024
* Swift Markdown

* wip

* remove cmark

* wip

* wip

* Update Sources/StyleguideV2/HTML/HTMLBuilder.swift

* wip

* Pin SQLKit for now

See: vapor/sql-kit#184

* revert

* wip
@gwynne
Copy link
Member

gwynne commented Sep 19, 2024

I need to look at this in more detail (now that I'm back on the grid) - I seem to remember changing this because the previous behavior didn't match either the documentation or behavior of JSONEncoder/JSONDecoder (on which the strategy options were originally modeled). But it's been long enough that I'm not entirely sure that was actually the case, I may have just flubbed it when I rewrote the relevant logic.

@gwynne
Copy link
Member

gwynne commented Sep 19, 2024

Nope, I was right the first time, I fixed it to match JSONDecoder's behavior:

Welcome to Swift version 6.0 (swift-6.0-RELEASE).
Type :help for assistance.
  1> import Foundation
  2> let d = JSONDecoder()
  3> d.keyDecodingStrategy = .convertFromSnakeCase
  4> struct Foo: Codable {
  5.     let userID: Int
  6. }
  7> try d.decode(Foo.self, from: Data("{\"user_id\":1}".utf8))
$E0: DecodingError = keyNotFound {
  keyNotFound = {
    0 = userID
    1 = {
      codingPath = 0 values
      debugDescription = "No value associated with key CodingKeys(stringValue: \"userID\", intValue: nil) (\"userID\")."
      underlyingError = nil
    }
  }
}
  8> struct Foo2: Codable {
  9.     let userId: Int
 10. }
 11> try d.decode(Foo2.self, from: Data("{\"user_id\":1}".utf8))
$R0: Foo2 = {
  userId = 1
}

(Note: Tested on both macOS and Linux with both Swift 5.10 and 6.0.)

@gwynne
Copy link
Member

gwynne commented Sep 22, 2024

@stephencelis I'm going to close this out as "works as expected", but if you have a counterargument, please feel free to reopen.

@gwynne gwynne closed this Sep 22, 2024
@stephencelis
Copy link
Author

stephencelis commented Sep 22, 2024

@gwynne This behavior change is breaking existing code in our code base when we attempted to upgrade.

It's been awhile since I opened this and was familiar with the problem, but if I remember correctly I believe the core of the issue was the snake case conversion changed:

Before:
  userID -> user_id

After:
  userID -> user_iD

@stephencelis
Copy link
Author

@gwynne OK rethinking about this I suppose it makes sense when it comes to roundtripping (though our app only cares about decoding at the moment). An unfortunate refactor awaits us some day 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants