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

Make aml_tester more ergonomic to use #142

Open
1 of 7 tasks
IsaacWoods opened this issue Feb 4, 2023 · 4 comments
Open
1 of 7 tasks

Make aml_tester more ergonomic to use #142

IsaacWoods opened this issue Feb 4, 2023 · 4 comments

Comments

@IsaacWoods
Copy link
Member

IsaacWoods commented Feb 4, 2023

At the moment, aml_tester works, but is pretty janky to use. Some improvements would be:

  • Correctly detect and error when iasl is not installed
  • Use one of those crates that makes panics more readable
  • Color code output
  • Provide summary at the end for each ASL file detected: PASS, COMPILE FAIL, PARSE FAIL
  • Allow testing only one ASL file (CLI -f option?)
  • Provide better control over when output from the parser is logged to console (e.g. only for failing test, separated out, etc.)
  • Provide CLI option to pass a chunk of AML instead (?might be better just adding a test in aml itself tbh)
@rw-vanc
Copy link
Contributor

rw-vanc commented Feb 8, 2023

What are your feelings on clap 4? Would you prefer derive or just update the clap code that's there? My suggestion is to allow files specified by --file or as positionals. --file can have multiple occurrences and be mixed with --path to specify order of parsing. Positionals would be parsed in the order given. --reset would cause namespace to be per table, no --reset would cause all files to be parsed into the same table.

@rw-vanc
Copy link
Contributor

rw-vanc commented Feb 9, 2023

It's easier to implement mixing of files and directories if they are the same arg name, or if they are just all positionals. What about dropping --files and --path, and just taking both file and path arguments as positionals? --path could be kept for backwards compatibility.

@IsaacWoods
Copy link
Member Author

No issues with updating clap, so have merged #149 - no strong feelings on derive vs not either, whatever you find most ergonomic.

I think it would be nicer to maintain two modes: one for parsing a directory of files independent of each other (the current setting), and another for parsing a set of tables in order in the same namespace. Can we pass an option (e.g. --parse_table_series or whatever) then a list of positional paths in the order of parsing to allow for this separate mode? I'm not too fussed which we make the default, as long as it's documented and both are possible explicitly.

Also no need generally to keep things backwards compatible with aml_tester, as it's entirely an internal testing tool atm afaik.

@rw-vanc
Copy link
Contributor

rw-vanc commented Feb 9, 2023

I have an implementation of using positionals. You can use --path DIR or positional FILE ..., but not both. I did not implement mixing files and dirs as positional, but I can if that's useful. File order is significant, aml files produced by compiling are parsed in the same order as their asl files in the original command line. Duplicates are removed, so the first occurrence of a file is the order used. There is a --reset option to create a fresh context after each table, the default is to reuse the context. I'm just doing some testing and will submit today.

Usage: aml_tester [OPTIONS] <--path <DIR>|FILE.{asl,aml}>

Arguments:
  [FILE.{asl,aml}]...  

Options:
      --no-compile  Don't compile asl to aml
      --reset       Clear namespace after each file
  -p, --path <DIR>  
  -h, --help        Print help
  -V, --version     Print version

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

No branches or pull requests

2 participants