Skip to content

Conversation

@quantum5
Copy link
Member

Fix all flake8 issues uncovered by #8878 except the one in #9200.

@quantum5 quantum5 requested review from kripken, sbc100 and tlively August 12, 2019 19:33
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Great!

How does this interact with #8878?

HANGUL_S_COUNT = 19 * 21 * 28

try:
unichr
Copy link
Member

Choose a reason for hiding this comment

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

indentation looks wrong here, and below

@quantum5
Copy link
Member Author

When used together with #8878, this reduces the list of python 3 flake8 errors. With #9200, there should be no errors left.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I'm not sure about all narrowing of the caught exceptions.. but lgtm in general.

try:
return json.loads(text)
except:
except ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, how can we be sure this is the only type of exception that can arise here?

Copy link
Member Author

Choose a reason for hiding this comment

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

From documentation:

If the data being deserialized is not a valid JSON document, a JSONDecodeError [subclass of ValueError] will be raised.

This is the only kind of error we want to recover from.

data[i] = encode_utf8(data[i])
return data
elif isinstance(data, unicode):
elif isinstance(data, type(u'')):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems strictly less readable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you prefer:

try:
  unicode
except NameError:
  unicode = str

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really. Just noticing out loud that this compatibility can make the code harder to read.

How would you do this in python3-only code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have written isinstance(data, str) here. But perhaps a better way is doing isinstance(data, bytes) so bytes gets returned raw, and everything goes through str and then encoded.

@quantum5 quantum5 requested a review from sbc100 August 12, 2019 20:48
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Can we add this to circleci?

@quantum5
Copy link
Member Author

Checking this on CircleCI is the intention of #8878.

@quantum5 quantum5 merged commit 872cc3e into emscripten-core:incoming Aug 12, 2019
@kripken
Copy link
Member

kripken commented Aug 13, 2019

@quantum5 looks like this caused breakage on MacOS, see

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket.appspot.com/8905225763363687616/+/steps/Emscripten_testsuite__upstream__other_/0/stdout

Looks like the change to except OSError: isn't enough, in macos_find_native_sdk_path.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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