Code cleanup #42
Code cleanup #42
Conversation
|
It looks clean to me. |
|
Please be sure to test the code and it'd be all good! :) |
|
😆 I will this afternoon. |
|
@santina I've gotten this to work on my system, and I cleaned up the appropriate code. There are still some things that need to be done, but they can be resolved in another major PR. |
|
Once this gets merged, I'll submit a PR for the debug logging that should be helpful for rebuilding the code base. |
|
|
||
| try: | ||
| stdout, stderr = recommended_pkgs.communicate(timeout=15) | ||
| except TimeoutExpired: |
There was a problem hiding this comment.
Is this the only exception we might hit?
There was a problem hiding this comment.
There certainly could be others. This particular exception handling block was just the most obvious one. For now I'll create an issue to help resolve this in future PRs.
| # Minor Version | ||
| minor, error = utils.system_r_call(rcmd_type="major", context=context) | ||
|
|
||
| context.R_version = "%s.%s" % (major, minor) |
There was a problem hiding this comment.
What if we actually got an error in the system_r_call? i.e. stdout is not actually the information we want. We should handle that.
There was a problem hiding this comment.
Will handle in future updates #49
| @@ -24,7 +24,7 @@ classifiers = [ | |||
| ] | |||
|
|
|||
| [tool.poetry.dependencies] | |||
| python = "^3.6" | |||
| python = ">=3.4" | |||
There was a problem hiding this comment.
I want to have more flexibility. In order to get this working on 3.4 I basically just removed F-string formatting and that gave us some backward compatibility. I think allowing 3.4-3.7 would be optimal. I changed this in rinse as well.
Using 2.7 would be good too, but I know 2.7 is going out of style next year. So that's probably not really worth our time.
| config_dict["R_VERSION"] = context.R_version | ||
| # Package lists | ||
| config_dict.update(__DEFAULT_CONFIG__) | ||
| pkg_lists = self.format_pkg_list(config_dict) | ||
| config_dict.update(pkg_lists) | ||
| config_dict.update(user_config) | ||
| logging.info(f"Config Dictionary: {config_dict}") | ||
| logging.info("Config Dictionary: %s" % config_dict) | ||
|
|
||
| # Dump the configuration dictionary to the YAML file in the R environment HOME | ||
| yaml.dump(config_dict, f, default_flow_style=False) |
There was a problem hiding this comment.
Could we remove all the commented-out code and instead open an issue and refer to them by issue number, just like what you have with See issue 2487 ?
There was a problem hiding this comment.
Sure that was actually copied from python's venv module. But that would probably be a good practice. For now, I'm going to leave it as is. My next PR will be a fairly extensive rework in order to break the functions up, and to handle the OS logic in separate classes (similar to what I just did in rinse; e.g. BaseInstallR, LinuxInstallR, MacInstallR, WindowsInstallR). I envision something like BaseRenvBuilder, LinuxRenvBuilder, MacRenvBuilder, and WIndowsRenvBuilder. These comments will be removed and appropriate issues will be created, if they are needed.
| context.env_bin_path = binpath = os.path.join(env_dir, binname) | ||
| context.bin_path = binpath | ||
| context.env_R_exe = os.path.join(binpath, r_exe) | ||
| context.env_R_script = os.path.join(binpath, r_script) | ||
| utils.create_directory(context.env_R_libs, self.clear) | ||
| logging.info(f"Environment R: {r_env_home}") | ||
| logging.info("Environment R: %s" % r_env_home) | ||
| return context | ||
|
|
||
| def create_configuration(self, context): |
There was a problem hiding this comment.
This function is huge. Could we break this down into smaller functions? Same go with some of the other functions.
There was a problem hiding this comment.
See my previous comment.
Thank you for the awesome review @santina. You caught some serious bugs that I overlooked. In the next PR, the code will be much easier to follow too! Let me know what you think! |
grabear
dismissed
santina’s stale review
I responded to all of your comments/suggestions. I need to go ahead and merge this so I can continue working on this.
|
I overrode the branch protection settings in order to push this through. Let me know if you have any questions. |
|
Sorry for not responding earlier. (go back in time and approve the change) |
No description provided.