nom icon indicating copy to clipboard operation
nom copied to clipboard

Implement Parser for tuples

Open kompass opened this issue 4 years ago • 11 comments

Hello ! My first PR to this project.

This PR does :

  • Implement the issue #1417.
  • Add some test.
  • Modify the fn tuple to use this new feature.

kompass avatar Oct 18 '21 19:10 kompass

that breaking change at this point there is no point to keep tuple trait with this change. I agree Tuple is obsolete now. I would tag this for nom 8.

if you want this PR accepted for nom 7 I would reverse the change on Tuple, and we could depreciate it in nom 7 et remove it in nom 8.

Stargateur avatar Oct 20 '21 21:10 Stargateur

I think the tuple function is still useful in case the tuple in question is not composed in another parser. Indeed, tuples implement Parser, not FnMut, so it is impossible to parse in the same way as other parsers. This code doesn't compile :

let (i, result) = (parser1, parser2)(i);

To maintain consistency in calls, I think it could be a good idea to keep it. The Tuple trait is now only used to be able to implement this function, I don't see any other way.

kompass avatar Oct 21 '21 09:10 kompass

I don't follow you, you want use it as documentation ? Unless I miss something remove Tuple trait the code still compile.

Stargateur avatar Oct 21 '21 09:10 Stargateur

The Tuple trait is still used to implement the tuple function.

We can still do :

let (i, result) = tuple((parser1, parser2))(i)?;

kompass avatar Oct 21 '21 11:10 kompass

The Tuple trait is still used to implement the tuple function.

We can still do :

let (i, result) = tuple((parser1, parser2))(i)?;

Since you change to trait Tuple {} this is impossible, do pub fn tuple<I, O, E: ParseError<I>, List: Parser<I, O, E>> it will compile fine.

Stargateur avatar Oct 21 '21 12:10 Stargateur

By doing this you allow any parser to be given as argument to tuple, and so you allow this code to compile :

let (i, result) = tuple(tag("hello"))(i)?;

The Tuple trait acts now like a marker, identifying parsers that really are tuples. Marker traits are not from my invention, some are defined in the std lib, like std::marker::Copy or std::cmp::Eq.

I am using this marker trait because for now, to my knowledge there is no other way to define a generic function on tuples of any size.

kompass avatar Oct 21 '21 12:10 kompass

That a valid point, but this lead to a simple conclusion tuple is simply not needed anymore. Tuple trait was actually the Parser trait, tuple() is just a parser that apply a Parser now. It's general enough and you want to limit it. Unless there is a technical reason to limit it I don't see why we should.

I see two outcome:

  • rename tuple() to parse() or apply(), allow a shortcut to (foo, bar).parse(input)
  • remove it

Stargateur avatar Oct 21 '21 13:10 Stargateur

I was considering the first solution too, I think it's a good idea! I will do that.

kompass avatar Oct 21 '21 13:10 kompass

Done. It's a real breaking change, since it removes a function and change the signature of some others.

kompass avatar Oct 21 '21 21:10 kompass

Done. It's a real breaking change, since it removes a function and change the signature of some others.

Well, Tuple was a pub trait since you change it, it was already a breaking change ^^. Big work indeed I will try to double check it.

Stargateur avatar Oct 21 '21 21:10 Stargateur

You are right, I forgot them ! It's fixed

kompass avatar Oct 25 '21 12:10 kompass

sorry for the delay, it is now merged! I removed the parse combinator as it is not really necessary, but the rest was great, thanks for your help!

Geal avatar Jan 02 '23 13:01 Geal