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

Turn folding an either into an option into an option.fromEither #64

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

thewilkybarkid
Copy link

@thewilkybarkid thewilkybarkid commented Feb 11, 2021

Closes #2520131

⚠️ The line above was added during the automatic migration of all github project issues to Kaiten. The old GH issue link is kept here for reference (it is now deprecated, continue follow the original issue on Kaiten).

Very much a work-in-progress while I get used to the code base, but this is an attempt to implement #61.

@kanbanbot kanbanbot added the WIP label Feb 11, 2021
Comment on lines 37 to 43
calleeIdentifier,
option.filter((callee) => callee.name === "fold"),
option.chain(flow((callee) => callee.parent, option.fromNullable)),
option.filter((parent): parent is TSESTree.MemberExpression => parent.type === AST_NODE_TYPES.MemberExpression),
option.map((parent) => parent.object),
option.filter((object): object is TSESTree.Identifier => object.type === AST_NODE_TYPES.Identifier),
option.exists((object) => object.name === 'either')
Copy link
Author

Choose a reason for hiding this comment

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

Currently looking for a way to determine if it's either.fold, even if it's E.fold or whatever.

Copy link
Member

Choose a reason for hiding this comment

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

Two ways:

  • use the type information (more accurate, but slower)
  • determine whether the "prefix" is imported from fp-ts (accurate enough, and faster because it's only a syntactic lookup)

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a closer look as soon as I have minute

isLazyValue(utils, "Option", "none"),
isCall(utils, "Option", "some")
])),
option.bind("namespace", flow(readonlyNonEmptyArray.head, findNamespace(utils))),
Copy link
Author

Choose a reason for hiding this comment

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

This isn't ideal, would rather use the imports than reading the code.

))
)

const isCall = <T extends TSESTree.Node>(utils: ContextUtils, module: Module, name: string) => (node: T) => pipe(
Copy link
Author

Choose a reason for hiding this comment

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

There’s a generic here due to type inference problems.

calleeIdentifier,
option.filter(hasName("constant")),
option.chain(typeOfNode),
option.exists(isFromModule("function"))
Copy link
Author

Choose a reason for hiding this comment

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

Could probably just check it’s from fp-ts rather than the type check.

@thewilkybarkid thewilkybarkid changed the title Start turning folding an either into an option into an option.fromEither Turn folding an either into an option into an option.fromEither Feb 17, 2021
@thewilkybarkid thewilkybarkid marked this pull request as ready for review February 17, 2021 10:49
@thewilkybarkid
Copy link
Author

(Guessing @kanbanbot doesn't respond to making PRs ready for review...)

@gabro
Copy link
Member

gabro commented Feb 17, 2021

@thewilkybarkid hehe, no that's very much a human process. There's quite a lot going on but I'll do my best to review this as soon as I have time :-) Thanks!

@thewilkybarkid
Copy link
Author

Ah, was wondering what it was as the account looks empty!

No rush with it, thanks.

@thewilkybarkid thewilkybarkid mentioned this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants