Export error types from brush_parser

[hwittenborn]

This allows the types to be used from functions like Shell::parse_string.

[reubeno]

I know these types are already all exported directly from brush_parser, but as a client of brush_core, are you specifically looking to inspect specific parse error types for differentiated error messages / handling?

The reason I ask is that we have future plans to overhaul the parsers in brush (#283 has some context) and I'm wondering if we need to clean up / better abstract some of the error types to anticipate that. Some of the error values directly wrap peg errors, which is very parser implementation specific right now. (Not ideal, but it grew very organically.)

Any chance it's mostly just brush_parser::ParseError that you need?

[reubeno]

I left a more general comment about exposing these, but with that said, I'd lean toward any re-exports of brush_parser types be done in a child module (e.g., brush_core::parser).

[hwittenborn]

On that note, would it be possible to just export the entirety of brush_parser as a parser module? This would help to avoid having to list multiple crates in my Cargo.toml configuration, especially since the two crates are intertwined with each other.

[reubeno]

I'd prefer starting with just exporting the types that are already directly passed through brush_core to clients of the latter. That includes ParseError and the entire brush_parser::ast module too. (And per the other thread, I'd like to see those published via a parser sub-module of brush_core.)

Re-exporting everything in brush_parser expands the compatibility contract (and ties the semver) of brush_core to brush_parser. With plans to revisit how the parser is implemented--and probably some need to clean up some of the exported helper functions in brush_parser, I think it's prudent to hold off on that.

Apart from the AST types and the error types, are you finding your code interacting with other parts of brush_parser directly?

[hwittenborn]

It's just the AST and ParseError type. I'd still be in favor of export brush_parser::* as brush_core::parser just so I can use the AST types and whatnot. We could export only the stuff I need from brush_parser's AST module, though there's so much that I don't see the point in simply not exporting the entire crate.

What do you think @reubeno?

[github-actions]

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
clone_shell_object	17.91 μs	17.99 μs	0.08 μs	⚪ Unchanged
eval_arithmetic	0.17 μs	0.17 μs	0.00 μs	⚪ Unchanged
expand_one_string	1.80 μs	1.86 μs	0.06 μs	⚪ Unchanged
for_loop	22.77 μs	22.41 μs	-0.36 μs	⚪ Unchanged
function_call	2.34 μs	2.32 μs	-0.02 μs	⚪ Unchanged
instantiate_shell	57.70 μs	57.01 μs	-0.69 μs	🟢 -1.20%
instantiate_shell_with_init_scripts	23804.94 μs	23912.78 μs	107.84 μs	🟠 +0.45%
parse_bash_completion	1676.54 μs	1709.68 μs	33.14 μs	🟠 +1.98%
parse_sample_script	1.87 μs	1.88 μs	0.00 μs	⚪ Unchanged
run_echo_builtin_command	15.01 μs	15.66 μs	0.65 μs	⚪ Unchanged
run_one_external_command	2349.09 μs	2071.92 μs	-277.17 μs	🟢 -11.80%
tokenize_sample_script	3.04 μs	2.84 μs	-0.20 μs	🟢 -6.46%

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
Overall Coverage	🟢 74.95%	🟢 74.95%	⚪ 0%

Minimum allowed coverage is 70%, this run produced 74.95%

Test Summary: bash-completion test suite

Outcome	Count	Percentage
✅ Pass	1303	61.78
❗️ Error	229	10.86
❌ Fail	186	8.82
⏩ Skip	376	17.83
❎ Expected Fail	15	0.71
📊 Total	2109	100.00

[hwittenborn]

Yep, it's just ParseError that I'm using currently. I wasn't aware of that overhaul, so we could definitely just export ParseError for now.

[reubeno]

Apologies that this PR has gotten a bid wedged; let's try to find some initial step we can move forward with. Here's my incremental proposal for now:

Suggested change

	pub use brush_parser::ParseError;
	/// Module containing the core parser facilities used and re-exported by this crate.
	pub mod parser {
	pub use brush_parser::{
	ParseError, ParserOptions, SourcePosition, Token, TokenLocation, TokenizerError, ast,
	};
	}

I think we can do better in the future, but I'm hoping this will give you a fair bit to go with. Do you think this will meet your needs for the time being, @hwittenborn?

[reubeno]

@hwittenborn I'd like to get back to this, and expand it a bit. Are you interested in picking this back up, or are you okay if I shepherd it in? (For reference, please see my draft #784.) We'll make sure to credit you on the work regardless.

[reubeno]

Update: I've rebased your changes, made some additions, and have submitted them as a separate PR (#784). I'll close this PR.