Command Launcher Update

[pderouen]

fixes #27

@ginty recommended using PathBuf it cleaned up a lot of the issues I was seeing. I don't know that all of the added OsString is necessary. In my testing I had better luck when using OsString. In any case the string has to be converted to the appropriate type for the system eventually. So, no harm (in my view).

@coreyeng can you kick the tires a bit?

I think the os.rs file becomes basically unused in this branch. I left it in for now.

[coreyeng]

Thanks! This is working okay for me so far. If we're not using os.rs anymore then that can be removed. The command handler there was really just to get something going on Windows.

[pderouen]

Sounds good. I will remove os.rs.

[pderouen]

on_windows and on_linux are being used by pyapi. So, I cleaned out the command launcher code instead of deleting os.rs.

[pderouen]

pushed an update that fixes #101

Do the Travis builds test the CLI commands?

[ginty]

Do the Travis builds test the CLI commands?

No, not yet

[ginty]

If you wanted to add some CLI tests I would add a cli_test.py here - https://github.com/Origen-SDK/o2/tree/master/example/tests

You should be able to switch dirs out of the app directory in Python code if you wanted to test global commands too.

Note that I think the naming of _test.py in the file is important, as is naming the test functions test_...

To invoke those tests run poetry run pytest for them all, or poetry run pytest tests/cli_test.py.
I think there are also ways to invoke specific tests only, though I'm not sure of the details.
The test framework is called pytest if you want to google it.

[pderouen]

If you wanted to add some CLI tests I would add a cli_test.py here - https://github.com/Origen-SDK/o2/tree/master/example/tests

It was just an honest question, wondering if the linux compile of the cli gets the tires kicked. I don't know that it's necessary to directly test the cli. As we're working on updates it'll be getting a constant run through. I'm willing to take a stab at a few tests to at least check that it doesn't die on origen v or origen g if you'd like to see that.

[coreyeng]

Partially from #97 , and partially from the mess I've got here, I think we'll need to rethink the command handler a bit before its all said and done. Might not be a bad idea to have some CLI tests to go off of. Not that it needs to be done now, just thinking out loud.

[ginty]

LGTM

[pderouen]

Yeah, I think I changed my mind about tests for the CLI over night. It would be better to have tests for it. I'll put a little thought into it over the next day or 2, but I think I'd rather merge these changes as is.

[pderouen]

This seems like a good way to test the cli:
https://rust-cli.github.io/book/tutorial/testing.html#testing-cli-applications-by-running-them

[pderouen]

@coreyeng part of adding testing I was thinking to add for the cli might involve moving the "do something with the args" code into a lib for direct unit testing... so instead of checking that origen g works when with a windows style path argument; I would just check that the strings passed to poetry are literals (and make that more likely by using a central fn or macro to build them).

Anyway, all of that to ask if you'd want to wait until after your branch is merged and I can work from there to add the testing? Or, what?

[pderouen]

I'm starting a cli_test branch to work on tests. I'll go ahead and merge this.