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

Delete unnecessary semicolons #459

Closed

Conversation

Projects
None yet
5 participants
@siyavash
Copy link
Contributor

commented Apr 5, 2019

Found unnecessary semicolons in ipynb files.

@siyavash siyavash requested review from lamberta and MarkDaoust as code owners Apr 5, 2019

@review-notebook-app

This comment has been minimized.

Copy link

commented Apr 5, 2019

Check out this pull request on ReviewNB: https://app.reviewnb.com/tensorflow/docs/pull/459

Visit www.reviewnb.com to know how we simplify your Jupyter Notebook workflows.

@googlebot googlebot added the cla: yes label Apr 5, 2019

@adammichaelwood
Copy link
Collaborator

left a comment

Thank you.

@yashk2810

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

If semicolon is not included, then this text gets printed "Text(0, 0.5, 'Sepal length')".

So, I think we need the semicolon.

If plt.show() is added, then we don't need the semicolon

@yashk2810 yashk2810 removed the ready to pull label Apr 6, 2019

@siyavash

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Oh, I didn't notice using semicolon in jupyter notebooks can suppress output of last line.
So, what's your suggestion ?
Add plt.show() or close this PR ?
IMO, it's better to add plt.show(), because in other parts of tutorial plt.show() is used.

@yashk2810

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

Adding plt.show() would work.

siyavash added some commits Apr 5, 2019

@lamberta
Copy link
Member

left a comment

Thanks!

TensorFlow-Docs-Copybara pushed a commit that referenced this pull request Apr 16, 2019

Copybara-Service
@lamberta

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Merged here: 18dec2e

@lamberta lamberta closed this Apr 16, 2019

@siyavash

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

I think this commit eed8604 is necessary, because in boosted_tree.ipynb, plt.show() is used before importing.

@lamberta

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Yes, I see.
Once the 'ready to pull' label is applied the merge process has started for the PR. I'll create a separate PR for that last commit.

@lamberta lamberta referenced this pull request Apr 18, 2019

Merged

Move up pyplot import #500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.