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

Bump dependencies, bugfixes, expand the descriptions a bit #77

Closed
wants to merge 1 commit into from
Closed

Conversation

pacak
Copy link
Contributor

@pacak pacak commented Jul 13, 2023

I also added a test that tries to feed parsers more unusual values such as --number=10 instead of --number 10 and reports results I consider strange. Not used anywhere yet, probably worth going over the examples and checking if desired behavior can be achieved.

Also fixed CI while I'm it.

@epage
Copy link
Contributor

epage commented Jul 13, 2023

This is a lot of different changes, each with their own aspects to discuss. I've gone ahead and merged or did my own version of some of them.

Still remaining

  • Testing
  • Feature description

Please split these out (or maybe start with an issue)

@pacak
Copy link
Contributor Author

pacak commented Jul 13, 2023

There are only tests left here and at the moment they are just running and reporting stuff in the console. Probably makes sense combine with stuff described in #9. Not sure what is the best way to represent that. Any objections if I replace python runner with something rust based?

@epage
Copy link
Contributor

epage commented Jul 14, 2023

If we add tests, I'd like them to be a bit more automated and fit within the existing practices of the project (e.g. python or Rust).

As for changing the runner, I'd recommend creating an issue on that so we can talk about what you are wanting to accomplish and requirements. I try to share as much of this code between projects as possible.

... "cargo script" would be nice for this

@epage
Copy link
Contributor

epage commented Jul 14, 2023

If this is just tests and they don't align with the project., I'm going to go ahead and close this. Feel free to create an issue for further discussing testing needs

@epage epage closed this Jul 14, 2023
@pacak
Copy link
Contributor Author

pacak commented Jul 14, 2023

Those are tests the same way as detecting invalid utf8 is a test.

  • does it support invalid utf8? Y/N
  • does it support --number=10 instead of --number 10?
  • does it support -vvv instead of -v -v -v?
  • does it detect unexpected flags?
  • does it detect known but expected flags?
  • does it support -n10 instead of -n 10?

etc.

At least those things that can be tested automatically and they all affect end user experience about the same time or more as not supporting OsString. Obviously automatic, rendering some kind of table. But if having more columns with separate description is not an option - probably nothing to do here.

@epage
Copy link
Contributor

epage commented Jul 14, 2023

Those are tests the same way as detecting invalid utf8 is a test.

invalid utf8 is automated and follows the patterns and practices of this repo. From what I could tell, these tests do not.

If we want to continue discussing this, please let's leave it to an issue.

@pacak pacak mentioned this pull request Jul 14, 2023
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.

2 participants