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

Pull request for #26: Allow CL arguments to be passed #46

Merged
merged 5 commits into from
Jan 23, 2017
Merged

Conversation

lboxell
Copy link
Contributor

@lboxell lboxell commented Jan 11, 2017

@arosenbe
Copy link
Contributor

arosenbe commented Jan 11, 2017

Very cool @lboxell! Nice work. I have a few comments, but none of them relate (strongly) to the code you've written so far.

  • The unit tests in you're asking us to pull are failing. I tried them locally and had the same different Errors. Can you fix that?
  • The template branch you're working on is a great idea. I think
    • We should add a step to show off the build_python command line argument functionality
    • Make the changes permanent in master so everyone can see how we do this. That would, of course, also require showing off the default argument passing as well.
  • Can you confirm that we'll never want to pass command line arguments to lyx or table builders?

@lboxell
Copy link
Contributor Author

lboxell commented Jan 11, 2017

@arosenbe, Per discussion:

  • Waiting on my congress_text run to finish before uninstalling my gslab_scons python module to re-run and fix the unit tests.
  • I think we should have at least one example in the template (since it isn't very intuitive), but showing it for all functions might be overkill.
  • I don't think we will.

@lboxell
Copy link
Contributor Author

lboxell commented Jan 12, 2017

@arosenbe,

I've updated the unit tests. One of the release_tools tests still fails for me and I'm not sure what the issue is. Given there are other tasks open regarding the release module and refactoring the unit tests, I'm inclined to leave as-is (or have someone who knows what is going on with those tests take a look at it).

My thoughts are that I'll update the template once I've updated the new gslab_scons module in master.

@lboxell
Copy link
Contributor Author

lboxell commented Jan 23, 2017

@arosenbe,

Just checking in on this.

@arosenbe
Copy link
Contributor

@lboxell, sorry for the delay.

I reran the tests after reinstalling a clean version of gslab_scons and didn't get any errors. Any thoughts on why you would have some that I wouldn't.

Your plan with the template sounds fine.

@lboxell
Copy link
Contributor Author

lboxell commented Jan 23, 2017

I updated my mock installation and re-ran the tests successfully.

I'm going to merge this branch now.

@lboxell lboxell merged commit 441fa9a into master Jan 23, 2017
@lboxell lboxell deleted the issue26_clarg branch January 23, 2017 22:38
@arosenbe
Copy link
Contributor

@lboxell, it looks like you didn't make a release with the change. Could you?

@lboxell
Copy link
Contributor Author

lboxell commented Jan 25, 2017

Done.

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