-
Notifications
You must be signed in to change notification settings - Fork 5
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
add otools-project local-odoo
#38
add otools-project local-odoo
#38
Conversation
463e6f4
to
2afb746
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG overall.
Few remarks and another one: can we have some tests pls? :)
odoo_tools/utils/proj.py
Outdated
odoo_src_path = build_path(get_conf_key("odoo_src_rel_path")) | ||
|
||
if not os.path.isfile(os.path.join(venv_dir, "bin", "odoo")): | ||
subprocess.call( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: any reason not to use https://github.com/camptocamp/odoo-project-tools/blob/master/odoo_tools/utils/os_exec.py#L9 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason, apart from not being aware of this utility function, and it does more than what is needed here. What would be the added value? I don't want to capture output.
Also I much prefer passing a pre split list of command line arguments to having a string passed to shlex.split, (note that subprocess also accepts a string for args, you just have to pass shell=True and it will call shlex.split for you)
https://docs.python.org/3/library/subprocess.html#frequently-used-arguments
changing the .proj.cfg broke the tests, I'll fix this. |
3ec74db
to
c217c83
Compare
@simahawk I welcome suggestions about what kind of tests you would like to see. This if very shell script-ish code and the amount of mocking which would be needed is a bit discouraging. |
1aac0c6
to
cd596d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super cool, thanks for adding tests!! 😗
|
||
def mock_run(args, stdout=None, **kwargs): | ||
call_spec = mock_spec.pop(0) | ||
if call_spec["args"] is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't be more clear to have the mock yield a an obj to test? Eg:
with patch("subprocess.run", mock_subprocess_run) as mocked:
# run your stuff
mocked.assert_args(...)
mocked.assert_sim_call(...)
It's just a bit hard to understand what is tested in the tests below just because everything is wrapped into the mocker.
However, I bet you had your reasons to go this way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I get you. You mean memorizing the args passed to the mocker and checking them after the whole thing ran ? Feasible, but then I don't think I can run my sim_call functions to produce the side effects (esp. creating a couple of files which are expected by further steps of the process, or producing output).
I'll think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming the command to otools-project checkout-local-odoo
Using verbs makes it clearer to understand what the command does
`otools-project local-odoo` is a new command to checkout odoo core + enterprise to the work directory at commit hashes matching the ones used in the docker image.
allow setting up a local virtual environment with odoo and the project installed.
the --venv controls the creation of the virtualenv, and --venv-path gives the name to use
aa8d959
to
9765145
Compare
@ivantodorovich done in 2bd8d4b |
otools-project local-odoo
is a new command to checkout odoo core + enterprise to the work directory at commit hashes matching the ones used in the docker image.