Skip to content
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

Implement str.expandtabs() based on CPython #4858

Merged
merged 7 commits into from Dec 10, 2019

Conversation

densmirn
Copy link
Contributor

No description provided.

@densmirn
Copy link
Contributor Author

Could you please queue the PR for review?

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Couple of comments to resolve then this is good to merge.

numba/unicode.py Outdated
thety = tabsize
if isinstance(tabsize, types.Omitted):
thety = tabsize.value
# if the type is optional, the concrete type is the captured type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# if the type is optional, the concrete type is the captured type
# if the type is optional, the concrete type is the captured type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

numba/unicode.py Outdated
found = False
for i in range(length):
code_point = _get_code_point(data, i)
if code_point == 9: # 0x9 '\t'
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity perhaps use variables in the outer scope like TAB = 0x9; LINEFEED = 0xa; CARRIAGE_RETURN = 0xd etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added such variables.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Nov 18, 2019
@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 18, 2019
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes.

@seibert
Copy link
Contributor

seibert commented Dec 3, 2019

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@densmirn
Copy link
Contributor Author

densmirn commented Dec 4, 2019

Could you please explain how can I fix the flake8 issue from CI?

@stuartarchibald
Copy link
Contributor

Could you please explain how can I fix the flake8 issue from CI?

I'd guess it's this line https://github.com/numba/numba/pull/4858/files#diff-a21787256ad779b4c4c5c99915c71025R2 and that master already has an import of sys present (

import sys
), so when this is merged into master (as what happens in the CI testing) there's duplication.

@stuartarchibald
Copy link
Contributor

Please could you resolve the conflicts by merging master into this branch. Thanks!

@densmirn
Copy link
Contributor Author

densmirn commented Dec 4, 2019

Resolved all the merge conflicts.

@seibert
Copy link
Contributor

seibert commented Dec 9, 2019

Resolved another merge conflict caused by merging the other string PRs. Waiting for Azure to confirm this PR is OK, then will merge.

@densmirn densmirn closed this Dec 10, 2019
@densmirn densmirn reopened this Dec 10, 2019
@densmirn
Copy link
Contributor Author

Resolved another merge conflict caused by merging the other string PRs. Waiting for Azure to confirm this PR is OK, then will merge.

There are no conflicts.

@seibert seibert merged commit 3ee0556 into numba:master Dec 10, 2019
@densmirn densmirn deleted the feature/str_expandtabs branch June 9, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants