Skip to content

Update transformer types and documentation #235

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

Closed
wants to merge 1 commit into from

Conversation

iby
Copy link
Contributor

@iby iby commented Sep 29, 2021

Compliments #212.

Comment on lines +64 to +66
column : {to?: fn, from?: fn}, // Transforms incoming or outgoing column names
value : {to?: fn, from?: fn}, // Transforms incoming or outgoing row values
row : {to?: fn, from?: fn}, // Transforms entire incoming or outgoing rows
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@porsager wasn't sure of the best way to represent this. Please tweak if have a better idea.

@iby iby force-pushed the chore/transformer branch from 23fc7da to 74a5b2a Compare September 29, 2021 15:15
types/index.d.ts Outdated
/** Transforms incoming and outgoing row values. */
value?: (value: any) => any | { from?: (value: any) => any, to?: (value: any) => any };
/** Transforms entire incoming and outgoing rows. */
row?: (row: postgres.Row) => any | { from?: (row: postgres.Row) => any, to?: (row: postgres.Row) => any };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@porsager is this the right signature for to?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assumes from's output should be to's input and vice versa, then I would suggest { from?: (row: postgres.Row) => any, to?: (row: any) => postgres.Row }, but I'm not sure where row.to could be used...

Also, I guess ( ) are missing in the union? TS sees (row: postgres.Row) => (any | { from?: (row: postgres.Row) => any, to?: (row: any) => any })

Suggested change
row?: (row: postgres.Row) => any | { from?: (row: postgres.Row) => any, to?: (row: postgres.Row) => any };
row?: ((row: postgres.Row) => any) | { from?: (row: postgres.Row) => any, to?: (row: any) => any };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Minigugus nicely spotted, thank you! Yes, I had the same feeling about the row. I updated my commit with your suggestions to avoid these micro-changes. Please feel free to review further.

@iby iby force-pushed the chore/transformer branch from 74a5b2a to 6da4d0f Compare September 29, 2021 18:06
@porsager
Copy link
Owner

@Minigugus is doing a big type update here #257, so closing this for now, but please comment on that PR if this fix is missing.

@porsager porsager closed this Jan 11, 2022
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.

3 participants