Pull request for gslab-econ/template#45: Standardise stata_executable by M-R-Sullivan · Pull Request #67 · gslab-econ/gslab_python

M-R-Sullivan · 2017-05-25T22:09:59Z

For gslab-econ/template#45. Specifically, I created this branch in response to this comment.

I should have made stata_executable our standard term for referring to users' Stata executables here (rather than stata_flavor, sf, user_executable, etc.).

arosenbe

Nice work @M-R-Sullivan! Just a few comments below.

arosenbe · 2017-05-26T15:23:12Z


-    Note: the user can specify a flavour by typing `scons sf=StataMP` 
-    (By default, SCons will try to find each flavour). 
+    Note: the user can specify a executable by typing `scons sf=StataMP` 


I think it might make sense to change the stata executable flag to se so that the abbreviation matches the new naming conventions.

This seems like a template-related comment, and I don't think it works with the current template. So I am just going to drop this comment. Let me know if you disagree!

arosenbe · 2017-05-26T15:31:18Z


    The function will check for user input in Scons env with
-    the flag e.g. `sf=StataMP-64.exe`. With no user input,
+    the flag e.g. `stata_executable=StataMP-64.exe`. With no user input,


This doesn't match the comments in build_stata.py.

I revised this function's doc string. Let me know if it needs further changes.

arosenbe · 2017-05-26T15:32:07Z

    # Fake scons-like env dict for misc.get_stata_executable(env)
-    fake_env = {'user_flavor': flavor} 
+    fake_env = {'stata_executable': executable} 
    stata_exec = get_stata_executable(fake_env)


Minor, but we refer to this object as stata_executable elsewhere. It might be worth a change.

arosenbe · 2017-05-26T15:37:20Z

+                    return executable
+
        elif sys.platform == 'win32':
            try:


It will be clearer to rewrite this try/except block as an if/else using os.path.keys().

arosenbe · 2017-05-26T15:41:37Z

@@ -142,7 +142,7 @@ def test_no_executable_in_path(self, mock_check, mock_path):
        mock_check.side_effect = fx.make_stata_side_effect('')


There's a typo in the docstring.

Good catch!

arosenbe · 2017-05-26T16:04:11Z

One more comment @M-R-Sullivan. Will you be making associated edits to standardize our naming in template?

M-R-Sullivan · 2017-05-30T22:49:05Z

@arosenbe - I made changes as suggested by my responses to your comments. I also standardised stata_executable in template in gslab-econ/template#45. Let me know what you think!

arosenbe · 2017-05-30T23:06:02Z

Looks good to me @M-R-Sullivan

* Pull request for #28: gslab_scons restructure (#47) * #28 modify check_code_extension() * #28 make distinction between `target_dir` and `log_dir` for all but `build_stata()`, add code flow comments in builder * #28 minor change, `return None` switched to `return False` for logical check * #28 add informative BadExecutableError messages * #28 merging remote to local * #28 add single log file + appending mechanism * #28 refactor build_stata() * #28 add formatting for logs * #28 fix typo in docstring * #28 update error messages and tests * #28 add mode check in start_log() * #28 change formatting in master tests to match work in this task * #47 dev commit to work from home * #47 rework logging different appending mechanism from builder logs to sconstruct.log * #28 respond to PR * #28 make pretty * #28 fix lyx pdf issue * #28 respond to more comments * #28 fix typo * Pull request for #59: Fix size warnings (#60) * #59: Redesigned size testing * #59: debugging * #59: additional work on list_ignored_files() * #59: Additional testing progress * #59: Brought tests to 100% coverage * change * #60: responded to PR * 60: response to PR * #59: brought size_warning tests to 100% coverage * #59: removed sys.exit() and raw_input() from size warning function * #59: scons debrief function * #59: altered size warnings * #59: small revisions to size warnings * #60: misc changes * Add note about sudo * Update README.md * Pull request for #43 in Template: Configuration tests (#66) * Migrates configuration tests * Preliminary tests setup * Additional tests * More edits * Adds more tests * Adds more tests * Adds more tests * Adds more tests * Adds more unit tests * Adds more unit tests * Fixes load_yaml_value tests * Dev commit * Dev commit * Brings coverage to 100 percent * Changes name * Edits tests for dropping --sf command line call * Changes name back * Fixes name in function * #66 Implements prelim PR comments * #66 Other PR comments * #66: Standardised exception classes * #66: updated some tests * #66: misc changes to tests * #66: some more changes * #66: changed error message * #66: removed extra newline from yaml creation * #66: aesthetic changes * #66: updated version number to 4 * Pull request for gslab-econ/template#45: Standardise stata_executable (#67) * gslab-econ/template#45 Changed to 'stata_executable' * Fixed a comment... * #67: Responses to PR * #67: Response to PR * Pull request for #58: Prepare for 4.0 (#65) * #58 cosmetic changes * Issue 58 - implement comments to builders 1) Fix R builder command line argument issue. Will test if issue persists for other builders. 2) Time-log failure is now kinder (does not block completion of SConstruct, just raise a noisy warning) * #58 small changes during merge * Pull request for #69: Update tablefill builder with 4.0 logging machinery (#70) * #69 add logging functionalities * #69 decrease clutter in sconscript.log * #69 prettify output * Pull request for #75: Print traceback for python builder (#76) * #75 cat \nTraceback to error message in Python builder * #75 remove new line * #75 fix crashing log.py if sconscript.log is empty * Pull request for #68: Remove Google Drive from default release (#72) * issue #68 - Fix broken imports in test - Move `read_yaml_value` to `misc` from `config` - Fix appropriate tests broken because of above move * #68 accommodate atexit implementation of state_of_repo * #68 fix numargs * #68 - instead of _ for user-config.yaml * #68 fix regex error in _release_tools * #68 respond to PR * #68 move check_and_expand_cache_path to misc.py, rename check_and_expand_path * #68 response to PR about release paths * #68 remove prompt for github token * #68 respond to PR * #68 add logical gate to use getpass.getpass for github_token * Pull request for #78: Updates readme (#79) * #78 Updates readme * Update README.md * Pull request for #73: Allows tablefill to use tex documents (#82) * #73 Preliminary implementation * #73 Updates info * #73 add newline after log for easy viewing * Pull request for #77: Replace <*> with <source file name> in <sconscript_*.log> (#80) * #77 add optional log extension through way of log_ext in ENV * #77 revert matlab reordering * #77 fix GitKraken merge typos * 77 add newline back in tablefill logging * #77 fix unittests * #77 respond to PR * #77 remove stray backslash * #77 fix broken unittests that allow dumb input as env * #83 add coverage to dependencies (#84) * Pull request for #85: Find way to speed up log.py (#87) * #85 log search with CL tool * #85 new log search works for mac * #85 get logging working on windows * #87 #85 var named cmd -> command, filter + map removes null paths, new warning msg * Update log.py * Update README.md * Update README.md * Update release.py * #88 format release instructions * #88 remove repeated word in readme

gslab-econ/template#45 Changed to 'stata_executable'

f35217d

M-R-Sullivan requested a review from arosenbe May 25, 2017 22:09

Fixed a comment...

885ef44

arosenbe reviewed May 26, 2017

View reviewed changes

M-R-Sullivan mentioned this pull request May 30, 2017

Pull request for #43: Implements 4.0 release comments gslab-econ/template#45

Merged

M-R-Sullivan added 2 commits May 30, 2017 15:12

#67: Responses to PR

5a41f66

#67: Response to PR

000c8dd

M-R-Sullivan merged commit e0249f1 into dev_four Jun 1, 2017

M-R-Sullivan deleted the template_pr45-standardize_stata_flavor branch June 1, 2017 21:42

M-R-Sullivan mentioned this pull request Jun 1, 2017

Implement 4.0 test comments for template gslab-econ/template#43

Closed

13 tasks

lboxell mentioned this pull request Jul 25, 2017

4.0.0 Release #81

Merged

		@@ -142,7 +142,7 @@ def test_no_executable_in_path(self, mock_check, mock_path):
		mock_check.side_effect = fx.make_stata_side_effect('')

Conversation

M-R-Sullivan commented May 25, 2017

Uh oh!

arosenbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arosenbe commented May 26, 2017

Uh oh!

M-R-Sullivan commented May 30, 2017

Uh oh!

arosenbe commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants