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

Remove u string prefixes #598

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Remove u string prefixes #598

merged 2 commits into from
Aug 20, 2020

Conversation

rahulporuri
Copy link
Contributor

This PR removes the u prefixes on strings. The prefixes dont do anything on Python 3 and they are unnecessary noise in the code.

Note that this PR is one PR in a series of PRs that aim to remove Python 2 support from the code base. This PR has been standalone given the volume of changes and to aid in the review process.

the u prefixes dont do anything on Python 3 and they are unnecessary
noise in the code

	modified:   docs/source/conf.py
	modified:   docs/source/config.rst
	modified:   examples/myapp.py
	modified:   traitlets/config/application.py
	modified:   traitlets/config/configurable.py
	modified:   traitlets/config/tests/test_application.py
	modified:   traitlets/config/tests/test_configurable.py
	modified:   traitlets/config/tests/test_loader.py
	modified:   traitlets/tests/test_traitlets.py
	modified:   traitlets/utils/tests/test_importstring.py
@Carreau
Copy link
Member

Carreau commented Aug 19, 2020

Thanks ! I'll have a look at try to get this in the next release ; We usually tend to avoid large scale PR that change all the file as it breaks git blame.

If this is not the only project you do that one; these changes can be automatically applied via https://github.com/asottile/pyupgrade

I'm also working on improving darker to apply upgrade only on changed lines which would have the advantage to progressively update the codebase w/o breaking git blame.

@@ -1229,7 +1229,7 @@ class AnyTraitTest(TraitTestBase):
obj = AnyTrait()

_default_value = None
_good_values = [10.0, 'ten', u'ten', [10], {'ten': 10},(10,), None, 1j]
_good_values = [10.0, 'ten', [10], {'ten': 10},(10,), None, 1j]
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice catch, pyupgrade would not find this !

@Carreau Carreau added this to the 5.0 milestone Aug 19, 2020
@Carreau
Copy link
Member

Carreau commented Aug 19, 2020

I have to run into a meeting but that looks good to me !

@rahulporuri
Copy link
Contributor Author

Thanks ! I'll have a look at try to get this in the next release ; We usually tend to avoid large scale PR that change all the file as it breaks git blame.

If this is not the only project you do that one; these changes can be automatically applied via https://github.com/asottile/pyupgrade

I'm also working on improving darker to apply upgrade only on changed lines which would have the advantage to progressively update the codebase w/o breaking git blame.

I've seen different strategies on different projects - some which prefer to make all of the stylistic upgrades in a single PR - squash merged into a single commit. That way, history is only broken in one commit. I can also understand/appreciate making modular changes only in parts of code/files in project which are modified in various PRs. I guess the downside with that approach is that some parts of the codebase are more readable than others - especially if the project as a whole moves slower in general.

I leave it up to y'all to decide what the strategy for traitlets is.

@Carreau
Copy link
Member

Carreau commented Aug 20, 2020

for new contribution we'll like not reject them, but I've been bitten enough by large changes and it breaking git-bisect that I'm usually on the fence.

Git blame now have the option to ignore some commit, but it's not avail via github...

Anyway thank, this is a great first contribution, it will make the 5.x codebase cleaner !

@Carreau Carreau merged commit a6d66cb into ipython:master Aug 20, 2020
@rahulporuri rahulporuri deleted the cln/remove-u-prefix-str branch August 20, 2020 14:06
@rahulporuri
Copy link
Contributor Author

thanks @Carreau

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