New release?

[elhabert]

Thank you for this great project!

I want to use some of the more recent features of grmtools (particularly, the addition of comments), but it is not available in a version published on crates.io, which might become a problem for me. Further, as these crates haven't seen activity in a while, it seems like a good time to cut a release?

Thank you in advance!

[ratmice]

The only really thing that is left up in the air on my part is #593
which I don't think is essential, it's kind of documented already as an experimental new feature, and 593 adds extra stuff to it.

I think perhaps it might be good to add an interim patch that reserves say the file extensions beginning with grm,

In the current state of the repository it treats all tests listed in the test_files directive of the %grmtools section
as raw test inputs. So if we make any extension like foo.grm*, print an error that the test_files beginning with the .grm prefix are reserved for future use.

That would allow us to delay #593 to a future release without any worries about compatibility (however unlikely!)
It also leaves us an escape hatch for after we've implemented e.g. .grmtest extension that we still have the rest of the .grm* extensions if we need additional incompatible format/extension changes.

So I think that change might be a good addition regardless of whether we delay #593 or not.
I'm still kind of having motivational issues on #593 regarding figuring out how to implement whitespace sensitivity.

Curious whether that sounds like a reasonable idea?

[ratmice]

The only other thing I can think of is a new cargo build.rs directive

https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargo-error
This appeared in 1.84 and we currently build with 1.85+, so we might want to make use of it,
or consider it for our next point release?

We might want to use that for error printing as they should be easier to read than the current panic text.
I'm not sure if we want to hold up a release for that?

There is another cargo directive in the works (for printing multi-line errors/warnings) that has been in theory accepted by the team, but hasn't made it into cargo yet.

In theory we can detect the version of cargo in use by using the $CARGO env var, and running $CARGO version.
Then using this new directive conditionally, It definitely seems like we will beat cargo to a release anyways.

Edit: My opinion on the stuff mentioned in this comment is that it all can wait, and be done in a future release (semver compatible with the upcoming one)

[ratmice]

The last thing I can think of is whether or not the test_files directive should accept an array or not,
currently it expects a single string/glob, it is probably worth making that decision before release.

Edit:
My opinion is I'm not sure it's really essential that we add it now, it feels like something that if we need it in the future
we could add it then, but people might want to set up their repository/testing with more test extensions in mind.

like:

test_files: "../tests/*"

or test_files: "test_*" with a trailing wildcard, so that it can pick up the new test extensions without renaming/changing repository layout or test filenames. We should still be able to allow an array like test_files: [".input", ".grmtest"]` in the future, if we feel comfortable with that kind of duck typing.

[ltratt]

I put this off... then forgot. Mea culpa! I'll never learn...

@ratmice I think your proposal is that we can make a 0.3.11 release now or do we need 0.4? I must admit that I've forgotten which things we put in that are semver breaking or not...

We might want to use that for error printing as they should be easier to read than the current panic text.
I'm not sure if we want to hold up a release for that?

I think we can do this at our leisure (I agree that it sounds nicer than our current approach, but it seems fully backwards compatible with semver). We could use the rustc_version crate for this, I think.

The last thing I can think of is whether or not the test_files directive should accept an array or not,
currently it expects a single string/glob, it is probably worth making that decision before release.

This is a good point. I think it should be an array.

We probably need to think about whether %grmtools sections are whitespace sensitive or not. I think we've implicitly assumed "not"; if so, we might as well be explicit. If that's the case, arrays will need [...] markers. If we're going to do that, we might as well put strings in "..." quotes, because we then reduce a classic ambiguity w.r.t. , and (space) characters in the name. I'm open-minded about this, though.

[ratmice]

I put this off... then forgot. Mea culpa! I'll never learn...

@ratmice I think your proposal is that we can make a 0.3.11 release now or do we need 0.4? I must admit that I've forgotten which things we put in that are semver breaking or not...

I think we added semver breakage, at least regarding the passing in of YaccKind, and LexerKind values being now optional because they can appear in the %grmtools section.

We might want to use that for error printing as they should be easier to read than the current panic text.
I'm not sure if we want to hold up a release for that?

I think we can do this at our leisure (I agree that it sounds nicer than our current approach, but it seems fully backwards compatible with semver). We could use the rustc_version crate for this, I think.

The last thing I can think of is whether or not the test_files directive should accept an array or not,
currently it expects a single string/glob, it is probably worth making that decision before release.

This is a good point. I think it should be an array.

👍 I'll start working on a patch

We probably need to think about whether %grmtools sections are whitespace sensitive or not. I think we've implicitly assumed "not"; if so, we might as well be explicit. If that's the case, arrays will need [...] markers. If we're going to do that, we might as well put strings in "..." quotes, because we then reduce a classic ambiguity w.r.t. , and (space) characters in the name. I'm open-minded about this, though.

Currently it should be whitespace insensitive, I also believe that various tests use different whitespace
(Some keep the %grmtools section all on a single line, others span multiple lines).
In the "authoratitive" grammar whitespace is completely dropped by the lexer.
https://github.com/softdevteam/grmtools/blob/master/lrpar/cttests/src/grmtools_section.test#L151

[ltratt]

Currently it should be whitespace insensitive, I also believe that various tests use different whitespace
(Some keep the %grmtools section all on a single line, others span multiple lines).

Indeed, that's what I was going by! So I think the array syntax should be either [a, b] or ["a", "b"]. I can see arguments for both, so please choose which ever you prefer!

[ratmice]

I've done some initial array syntax stuff (totally untested), it uses ["a", "b"], since the existing strings are using " it doesn't actually touch the string parsing.

It seems unlikely I'll finish it up today and if not, I won't be able to return to this until the middle of next week.
What is left to be done is to make the hand-written GrmtoolsSectionParser::parse_value function in cfgrammar/src/header.rs accept the array syntax (and split it up to separate out key : value parsing into their own functions), and make the test_files key expect arrays of strings rather than strings.

The branch is here, if anyone can't wait, or wants to pick this up and finish it before I can, that would be fine too.
But if not I'll return to it when I can.

https://github.com/ratmice/grmtools/tree/grmtools_section_array_values

[ratmice]

We might want to use that for error printing as they should be easier to read than the current panic text.
I'm not sure if we want to hold up a release for that?

I think we can do this at our leisure (I agree that it sounds nicer than our current approach, but it seems fully backwards compatible with semver). We could use the rustc_version crate for this, I think.

When I woke up, it had occurred to me that rustc_version may not be exactly what we want here,
in particular the build.rs executable sets a env var $CARGO, and then we can execute $($CARGO version).

I haven't tested, but I believe that when using a rust-toolchain.toml you can select a version of rustc which would differ from the version of cargo in either direction newer compiler (old cargo) or older complier (new cargo).

I don't know of a crate for cargo version, but it shouldn't be too hard to throw something together if we'd like to.

Edit: I don't think trying to execute $($CARGO version) is great (not sure what else to do really), however I don't think it will affect any current wasm builds (on that I'm not totally certain), because I think build.rs is still built for the build machine, and none of the rustc/cargo build machine tools run under wasm. In theory if that does become a problem it's easy to disable autodetection with a feature or some cfg setting.

[ltratt]

I'm a bit nervous about cargo version execution in part because I feat that there'll be some weird environment where it doesn't work, in ways that will bamboozle us e.g. cross compiling; I think I'd try seeing how the "rustc_version is a proxy for cargo version" thing works first.

[ratmice]

It probably isn't actually worth worrying about right now, cargo:error is available in 1.84 (and grmtools requires 1.85 so we can use that unconditionally), and the multi-line error message work hasn't been done nor do we know when it'll be available, so we can just deal with it at that time.

I guess my concern is that with executing cargo version we know there are no false positives, that we will emit a directive cargo doesn't understand. If executing it doesn't work, the worst case is we fall back to the current code path.
While with rustc_version as a proxy, it seems possible we can emit directives that cargo might not understand.

I haven't worked out the exact effect of what emitting such a directive might be, I would guess that it'd fail immediately and in the multi-line case potentially with a incomplete error message (the first line of the error).

Anyhow, this all probably belongs in it's own issue I don't think we need to hash that all out (now, or here).

[ltratt]

@ratmice Agreed on the versioning aspect: we can punt on that for now.

I have quickly checked your branch; my only initial comment is that none of the tests use more than one item in an array :)

[ratmice]

Yeah, the branch is still in an awkward place where nothing passes because the parsers are out of sync.

But after that is fixed yeah, we don't currently have anywhere that tests multiple globs nor really anywhere to put such tests. The current tests of test_files are all the lrpar/examples. (I don't think adding multiple globs to lrpar/examples seems like the right thing to do in the sense that these are mostly user examples)

We could perhaps add some sort of extra_files dictionary to the cttests yaml that would be output along side the parsers in the build directory, then have a cttest that does a couple globs with the extra_files filenames in the key?

[ltratt]

We could perhaps add some sort of extra_files dictionary to the cttests yaml that would be output along side the parsers in the build directory, then have a cttest that does a couple globs with the extra_files filenames in the key?

Good idea!