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 fold_count parser equivalent for count #1401

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

Conversation

nickelc
Copy link
Contributor

@nickelc nickelc commented Sep 16, 2021

No description provided.

@cenodis
Copy link
Contributor

cenodis commented Sep 16, 2021

This is already possible with fold_many_m_n. I dont think adding a completely new parser just for m == n is worth it.

Comment on lines +662 to +665
Ok((i, o)) => {
acc = fold(acc, o);
input = i;
}
Copy link
Contributor

@cenodis cenodis Sep 16, 2021

Choose a reason for hiding this comment

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

There needs to be a check inside this loop to see if the child parser actually consumed any input. The child parser not consuming input inside a loop is an error condition. See fold_many_m_n for an example.

@nickelc
Copy link
Contributor Author

nickelc commented Sep 17, 2021

This is already possible with fold_many_m_n. I dont think adding a completely new parser just for m == n is worth it.

You could say the same about count.

@cenodis
Copy link
Contributor

cenodis commented Sep 17, 2021

You could say the same about count.

There are some convenience parsers that allow you to avoid using fold_* (such as many_*). But I think thats a bit different from adding another fold_* parser.

My current problems with this:

  • We already have 3 fold_* functions and adding another one for a use case thats already covered by another (and is trivial to use for such cases) is just unnecessary .
  • Its name doesnt fit in with the other functions of the fold_* family. If anything it should be called fold_many_m.

@Stargateur
Copy link
Contributor

At least name it fold_many_n

@cenodis
Copy link
Contributor

cenodis commented Sep 17, 2021

At least name it fold_many_n

I think you mean fold_many_m. The n is the second parameter which doesnt exist here.

@Stargateur
Copy link
Contributor

At least name it fold_many_n

I think you mean fold_many_m. The n is the second parameter which doesn't exist here.

in math, n come first then you use m, plus n is the max in fold_many_m_n.

@cenodis
Copy link
Contributor

cenodis commented Sep 17, 2021

in math, n come first then you use m, .

I dont think thats a set rule. n is often used to denote a number of something. However in math one also often uses x or even a. It usually comes down to preference and context. To my knowledge there is no strictly enforced order of variable names, there are some conventions but those are not mandatory and only apply in their respective context.
In this case I would prefer to stay in line with the other function names and make it fold_many_m. Alternatively we could drop it altogether and just make it fold_many, with the possibility to later expand the parameter to also take ranges as proposed in #1393.

plus n is the max in fold_many_m_n

Irrelevant. This parser is about the case of m == n.

@Stargateur
Copy link
Contributor

Irrelevant. This parser is about the case of m == n.

how irrelevant, that stupid, obviously this is for max only so take n is totally relevant.

Alternatively we could drop it altogether and just make it fold_many

yes why not

@cenodis
Copy link
Contributor

cenodis commented Sep 17, 2021

how irrelevant, that stupid, obviously this is for max only so take n is totally relevant.

The only obvious thing is that n and m are the same in this parser. So they can be used interchangeably. Whether you use the minimum or the maximum is irrelevant if they are the same.

@Stargateur
Copy link
Contributor

Stargateur commented Sep 17, 2021

how irrelevant, that stupid, obviously this is for max only so take n is totally relevant.

The only obvious thing is that n and m are the same in this parser. So they can be used interchangeably. Whether you use the minimum or the maximum is irrelevant if they are the same.

I will repeat so, no that different you don't understand. if you take m this would mean only minimum, so take_many_m would be read as "take at least m" cause m IS the minimum in fold_many_m_n, n is more logical ( n come before m), match the max from already exist function and is often used as "the number n".

@cenodis
Copy link
Contributor

cenodis commented Sep 18, 2021

This case is also handled by #1402 which in my opinion is a much more elegant solution compared to adding yet another specialized parser.

@Xiretza
Copy link
Contributor

Xiretza commented Jan 2, 2023

This should be solved by #1608, which allows fold(n, parser, init, fold).

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

5 participants