Skip to content

MNT Clean up examples #21246

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2021
Merged

MNT Clean up examples #21246

merged 1 commit into from
Oct 14, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Oct 5, 2021 •
edited
Loading

What does this implement/fix? Explain your changes.

See #21234 (comment).

  • chmod -x
  • remove Python shebang from examples: #!/usr/bin/python
  • only UTF-8-encoded files contain "-# -*- coding: utf-8 -*-"

Any other comments?

Examples could be made more consistent.

  1. What about -# -*- coding: utf-8 -*-? Keep it in UTF-8-encoded files? Remove it from all example files? Add it to all example files?
  2. Some examples have a print(__doc__), others don't. Should it be added to all examples?
  3. The authors/license are not consistent. Do something about it?

thomasjpfan
thomasjpfan reviewed Oct 6, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @DimitriPapadopoulos !

I agree with removing #!/usr/bin/python.

As for print(__doc__), I am +1 on having it everywhere, right under the files' docstring. I think it came from a time where all the narrative was in the file's docstring. Now we have examples with multiple cells and many view the examples as HTML. I think there is still value in printing out the docstring, so there is some feedback running the example locally. Note I think print(__doc__) can be another PR, so we do not block this one.

examples/applications/plot_cyclical_feature_engineering.py Outdated
@@ -1,3 +1,4 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

From my understanding, utf-8 is the default encoding for Python 3. Given that, I would prefer to remove it everywhere. @DimitriPapadopoulos What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware or had forgotten UTF-8 is the default encoding of source code files in Python 3:

If no encoding declaration is found, the default encoding is UTF-8.

So yes, all these encoding declarations should be removed. They just obfuscate examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put back the encoding declarations for consistency with this other PR: #21260.

@DimitriPapadopoulos
Copy link
Contributor Author

I would also prefer a different PR for print(__doc__).

@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Oct 6, 2021
Merged
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 7, 2021 •
edited
Loading

@thomasjpfan @jjerphan See also #21260. Should I give up on the # -*- coding: utf-8 -*- stuff? I am obviously in favour of removing the encoding declaration, especially in examples because it obfuscates them, but then we need to be consistent over the whole repo. However, I am even more in favour of a quick merge of this PR so that I can work on a new print(__doc__) PR - even at the expense of skipping the # -*- coding: utf-8 -*- stuff in this PR.

@thomasjpfan
Copy link
Member

Should I give up on the # -- coding: utf-8 -- stuff?

Yea let's revert the coding: utf-8 lines. I think this PR can still safely remove the #!/usr/bin/python.

@DimitriPapadopoulos
MNT Clean up examples
696a125 
* chmod -x
* remove Python shebang from examples: #!/usr/bin/python
@DimitriPapadopoulos
Copy link
Contributor Author

I've reverted all # -*- coding: utf-8 -*- changes.

thomasjpfan
thomasjpfan approved these changes Oct 8, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Oct 12, 2021
Merged
@rth
Copy link
Member

rth commented Oct 14, 2021

Thanks!

@rth rth merged commit 5f6e17c into scikit-learn:main Oct 14, 2021
@DimitriPapadopoulos DimitriPapadopoulos deleted the examples_shebang branch October 14, 2021 18:28
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
Merged
10 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
@DimitriPapadopoulos @glemaitre
MNT Remove #!/usr/bin/python shebang in examples (scikit-learn#21246)
998465a
glemaitre pushed a commit that referenced this pull request Oct 25, 2021
@DimitriPapadopoulos @glemaitre
MNT Remove #!/usr/bin/python shebang in examples (#21246)
e4f74fc
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@DimitriPapadopoulos @samronsin
MNT Remove #!/usr/bin/python shebang in examples (scikit-learn#21246)
10d8efb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DimitriPapadopoulos @thomasjpfan @rth @TomDLT