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

make AST.pick correctly handle key renames #2511

Merged
merged 1 commit into from
Apr 13, 2024
Merged

make AST.pick correctly handle key renames #2511

merged 1 commit into from
Apr 13, 2024

Conversation

gcanti
Copy link
Contributor

@gcanti gcanti commented Apr 13, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Related

Copy link

changeset-bot bot commented Apr 13, 2024

🦋 Changeset detected

Latest commit: cacd6ef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@effect/schema Minor
@effect/cli Major
@effect/experimental Major
@effect/platform-browser Major
@effect/platform-bun Major
@effect/platform-node-shared Major
@effect/platform-node Major
@effect/platform Major
@effect/rpc-http Major
@effect/rpc Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@gcanti gcanti added the schema label Apr 13, 2024
@gcanti gcanti merged commit 0d3231a into main Apr 13, 2024
12 checks passed
@gcanti gcanti deleted the fix/2510 branch April 13, 2024 08:46
@github-actions github-actions bot mentioned this pull request Apr 13, 2024
@nspaeth
Copy link
Contributor

nspaeth commented Apr 13, 2024

It looks like using the new name for the key is explicitly dis-allowed. Is the reasoning for this that there is no way to determine which key the new key was renamed from in the type signature? If there is a way to change that I think it would be a big DX improvement.

I imagine that in almost every case, if a developer has chosen to rename a key they expect to refer to it that way from then on, so this is really confusing. if after seeing that it doesn't typecheck and they have to use the old name, I expect that they are likely to be suspicious that it is a bug rather than an intentional descision because it is so counter-intuitive.

const someSchema = S.struct({
  a: S.propertySignature(S.string).pipe(S.fromKey('b')),
  b: S.number,
})

const schema = pipe(
  someSchema,
  S.omit('a') // <-- Seems like the expected usage
)

@gcanti
Copy link
Contributor Author

gcanti commented Apr 13, 2024

this schema is illegal and raises a Duplicate property signature "b" error

const someSchema = S.struct({
  a: S.propertySignature(S.string).pipe(S.fromKey("b")),
  b: S.number
})

@gcanti
Copy link
Contributor Author

gcanti commented Apr 13, 2024

let's change S.fromKey("b") to S.fromKey("c")

import * as S from "@effect/schema/Schema"

const opaque = S.struct({
  a: S.propertySignature(S.string).pipe(S.fromKey("c")),
  b: S.number
})

/*
const schema: S.Schema<{
    readonly a: string;
    readonly b: number;
}, {
    readonly c: string;
    readonly b: number;
}, never>
*/
const schema = S.asSchema(opaque)

const p = schema.pipe(S.omit("a"))

what do you expect p to look like?

@nspaeth
Copy link
Contributor

nspaeth commented Apr 13, 2024

let's change S.fromKey("b") to S.fromKey("c")

Yes, sorry. Made an error when simplifying from a local test.

what do you expect p to look like?

I think I'd expect:

const p = schema.pipe(S.omit("a"))
type p = S.Schema<{ readonly b: number; }, { readonly b: number; }, never>

const q = schema.pipe(S.pick("a"))
type q = S.Schema<{ readonly a: number; }, { readonly c: number; }, never>

@gcanti
Copy link
Contributor Author

gcanti commented Apr 13, 2024

That would be possible at runtime (since the underlying ast still has all the information), but it's a problem at the type-level as there's nothing anymore linking the a prop with the c prop.

@nspaeth
Copy link
Contributor

nspaeth commented Apr 13, 2024

I wonder if it is possible to create some kind of sentinal type:

type opaque = S.Schema<{
  readonly a: string;
  readonly b: number;
}, {
  readonly c: Renamed<'a', string>;
  readonly b: number;
}, never>


// S.Schema.Encoded<opaque>
type opaqueEncoded = {
  readonly c: string;
  readonly b: number;
}

// S.Schema.Type<opaque>
type opaqueEncoded = {
  readonly a: string;
  readonly b: number;
}

@gcanti
Copy link
Contributor Author

gcanti commented Apr 13, 2024

yeah it might be possible, but it complicates things and I'm not sure it's worth it, when working with structs I would suggest the following workaround

import * as S from "@effect/schema/Schema"
import { omit, pick } from "effect/Struct"

const schema = S.struct({
  a: S.propertySignature(S.string).pipe(S.fromKey("c")),
  b: S.number
})

const p = S.struct(omit(schema.fields, "a"))

const q = S.struct(pick(schema.fields, "a"))

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

Successfully merging this pull request may close these issues.

None yet

2 participants