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

Rename context combinators #454

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

fowenix
Copy link
Contributor

@fowenix fowenix commented Jun 20, 2023

No description provided.

@bew
Copy link
Contributor

bew commented Jun 20, 2023

Why?
Was it discussed previously?

@zesterer
Copy link
Owner

Hmmm, what value does this rename bring?

@fowenix
Copy link
Contributor Author

fowenix commented Jun 20, 2023

I felt like ignore_with_ctx and then_with_ctx weren't the best names for what they do.
But after some more thought, I now think then_with_ctx is fine but ignore_with_ctx feels off

@fowenix
Copy link
Contributor Author

fowenix commented Jun 20, 2023

Maybe renaming ignore_with_ctx into then_into_ctx where this shows that the ctx is being consumed, while then_with_ctx shows it is being group by with

@zesterer
Copy link
Owner

zesterer commented Jun 20, 2023

ignore_with_ctx (which doesn't generate the output of the first parser) is intended to be a parallel with ignore_then (which also doesn't generate the output of the first parser).

a.ignore_with_ctx(b) // Produces b's output
a.ignore_then(b) // Produces b's output

I agree that it's maybe a little awkwardly named though. What do you think about the following?

// The existing parsers (not changing)
a.ignore_then(b)
a.then(b)

// Their context-passing equivalents
a.ignore_then_ctx(b)
a.then_ctx(b)

@zesterer
Copy link
Owner

Oh I didn't mean that as an instruction, just as an opening point of discussion. I'm not convinced by the names I suggested myself.

@fowenix
Copy link
Contributor Author

fowenix commented Jun 20, 2023

a.ignore_then_ctx(b)
a.then_ctx(b)

IMO this is the most convincing and asking ChatGPT to generate some alternatives names didn't give any viable ones.

@zesterer
Copy link
Owner

Well a problem with .then_ctx is that it implies that the second part is the context, which it isn't. The 'most correct' names would probably be .ctx_then and .ctx_ignore_then, and are potentially a little easier to quickly spot as context-sensitive combinators.

Regardless, I think this is something better left to an issue with discussion for the time being.

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.

None yet

3 participants