Skip to content

Fix errors in syncing logic and update to use subprocess.check_call instead os.system#334

Merged
wlach merged 3 commits intomainfrom
add-missing-space
Sep 2, 2021
Merged

Fix errors in syncing logic and update to use subprocess.check_call instead os.system#334
wlach merged 3 commits intomainfrom
add-missing-space

Conversation

@wlach
Copy link
Contributor

@wlach wlach commented Sep 2, 2021

No description provided.

@wlach wlach requested a review from jklukas September 2, 2021 14:19
f"--delete "
f"--exclude index.html"
f"--exclude index.html "
f"--content-type 'application/json' " + sync_params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably also a problem that needs to be fixed. See https://github.com/mozilla/probe-scraper/pull/327/files#r701131309

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, am pretty sure this is fine:

wlach@antwerp glean-dictionary % python3
Python 3.8.2 (default, Jun  8 2021, 11:59:35) 
[Clang 12.0.5 (clang-1205.0.22.11)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.system("echo " + ("foo"))
foo
0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mayb to just circumvent the whole "manually adding spaces" problem, stuff all arguments into an array, then " ".join() that together?
(Doesn't os.system alternatively take an array to do this for you?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subprocess module provides more comprehensive facilities, including taking in a list of args instead of a full string. I think we should transition this code to do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, am pretty sure this is fine

That example hits a python gotcha. ("foo") with a single entry and no trailing comma is not interpreted as a tuple. The example fails if you use ("foo",):

>>> os.system("echo " + ("foo",))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate str (not "tuple") to str

Copy link
Contributor Author

@wlach wlach Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subprocess module provides more comprehensive facilities, including taking in a list of args instead of a full string. I think we should transition this code to do that.

Yeah, I was just thinking that. I'll update this PR to use check_call

@wlach wlach changed the title Add missing space to sync command Fix error in syncing logic and update to use subprocess.check_call instead os.system Sep 2, 2021
@wlach wlach changed the title Fix error in syncing logic and update to use subprocess.check_call instead os.system Fix errors in syncing logic and update to use subprocess.check_call instead os.system Sep 2, 2021
@wlach wlach requested a review from jklukas September 2, 2021 14:44
@wlach wlach enabled auto-merge (squash) September 2, 2021 14:55
@wlach wlach merged commit 4732e28 into main Sep 2, 2021
@wlach wlach deleted the add-missing-space branch September 2, 2021 14:59
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.

3 participants