Skip to content

Use symbolic names for SEEK_SET, etc, instead of hardcoding them#12057

Merged
methane merged 1 commit into
python:masterfrom
ngie-eign:use-symbolic-names-for-seek-api
Mar 3, 2019
Merged

Use symbolic names for SEEK_SET, etc, instead of hardcoding them#12057
methane merged 1 commit into
python:masterfrom
ngie-eign:use-symbolic-names-for-seek-api

Conversation

@ngie-eign
Copy link
Copy Markdown
Contributor

The previous code hardcoded SEEK_SET, etc. While it's very unlikely
that these values will change, it's best to use the definitions to avoid
there being mismatches in behavior with the code in the future.

Signed-off-by: Enji Cooper yaneurabeya@gmail.com

The previous code hardcoded `SEEK_SET`, etc. While it's very unlikely
that these values will change, it's best to use the definitions to avoid
there being mismatches in behavior with the code in the future.

Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
@ngie-eign
Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka: while it isn't directly tied to the issue discussed in https://bugs.python.org/issue36111, it is something that (when inspecting the code), I thought should be changed. I remember looking at this code in the past and thinking similar things, IIRC.

@serhiy-storchaka
Copy link
Copy Markdown
Member

I am not sure this PR should be merged. Once I wrote much larger patch which replaced literal numbers with named constants SEEK_*, but abandoned it because I have not seen the benefit from such code churn. This PR is smaller, but it makes the use of literals and names inconsistent even in the same file.

In any case this change introduced a bug: missed break in one of cases.

@ngie-eign
Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka: crap. I had fixed that before.. not sure why it wasn’t in the merge content :(..

@ngie-eign ngie-eign deleted the use-symbolic-names-for-seek-api branch March 3, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants