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

Added metadata to legends.py #12920

Merged
merged 6 commits into from
Mar 16, 2023
Merged

Added metadata to legends.py #12920

merged 6 commits into from
Mar 16, 2023

Conversation

chinmaychahar
Copy link
Contributor

Added metadata to legends.py in examples/models/legends.py

@bryevdv
Copy link
Member

bryevdv commented Mar 8, 2023

Hi @chinmaychahar FYI it is not necessary to close PRs and open new ones to make changes. You can make and commit new changes in the existing branch, and then git push them from the same branch. Then GitHub will update the existing PR. This is preferable, so that that there is a chain of continuity for reviews.

@bryevdv bryevdv mentioned this pull request Mar 8, 2023
1 task
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #12920 (7bd9857) into branch-3.2 (2739e10) will increase coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           branch-3.2   #12920   +/-   ##
===========================================
  Coverage       92.35%   92.36%           
===========================================
  Files             315      315           
  Lines           19936    19962   +26     
===========================================
+ Hits            18411    18437   +26     
  Misses           1525     1525           

@chinmaychahar
Copy link
Contributor Author

chinmaychahar commented Mar 8, 2023

Hey @bryevdv, thanks for mentioning that. I actually had to delete the previous fork because there were some issues with the local setup. That's why deleting and creating the new fork closed the PR and I had to reopen a new one. Hope that's okay

@bryevdv bryevdv requested a review from tcmetzger March 8, 2023 06:15
@bryevdv
Copy link
Member

bryevdv commented Mar 8, 2023

@chinmaychahar 👍 no worries, FYI @tcmetzger has indicated he should be able to review this PR by the end of the week

Copy link
Member

@tcmetzger tcmetzger 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! I only have two small requests!

examples/models/legends.py Show resolved Hide resolved
examples/models/legends.py Outdated Show resolved Hide resolved
@chinmaychahar
Copy link
Contributor Author

I've made the changes accordingly. Please let me know if I missed on something

@bryevdv
Copy link
Member

bryevdv commented Mar 11, 2023

HI @chinmaychahar this is almost ready, there are just some linter issues to deal with:

E        File contains trailing whitespace: examples/models/legends.py, line 1.
E         File contains trailing whitespace: examples/models/legends.py, line 2.
E         File contains trailing whitespace: examples/plotting/tap.py, line 2

That last one is not your fault, and is fixed in another PR, but the simplest thing here is probably just to delete the trailing space in your branch.

E         examples/models/legends.py:5:1: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
E         examples/models/legends.py:5:3: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
E         examples/models/legends.py:5:166: E501 Line too long (175 > 165 characters)
E         examples/models/legends.py:6:1: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
E         examples/models/legends.py:6:3: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)

it looks like some unicode spaces crept in an should be replaces with a basic space character, also the apis line is too long. I would suggest removing one or two of the references. Eventually we will hopefully auto-reference everything and won't need these manual references anyway.

@chinmaychahar
Copy link
Contributor Author

@bryevdv Thanks for the review, I made the changes.

@bryevdv
Copy link
Member

bryevdv commented Mar 13, 2023

@chinmaychahar There is still a unicode space issue tripping up the linter:

FAILED tests/codebase/test_ruff.py::test_ruff - AssertionError: ruff issues:
  examples/models/legends.py:5:1: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
  examples/models/legends.py:5:3: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
  examples/models/legends.py:6:1: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
  examples/models/legends.py:6:3: RUF002 [*] Docstring contains ambiguous unicode character ` ` (did you mean ` `?)
  Found 4 errors.
  [*] 4 potentially fixable with the --fix option.

I think if you run ruff --fix . in the top level of the repo, it might fix those issues for you (and then you can commit and push the changes that ruff made)

@chinmaychahar
Copy link
Contributor Author

@bryevdv there were some rebase issues but I executed ruff --fix . this time, like you suggested and hopefully it should it work.

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

@chinmaychahar I was about to merge but then I noticed the unrelated changed to examples/styling/mathtext/latex_schrodinger.py are in this PR for some reason. Can you remove that change? We should keep PRs on-topic, also changing those colors will require generating new thumbnails so that should all happen in a dedicated PR.

@chinmaychahar
Copy link
Contributor Author

@bryevdv yes, you're right. I've removed those changes from this branch

@chinmaychahar chinmaychahar requested review from bryevdv and removed request for tcmetzger March 16, 2023 08:00
@bryevdv
Copy link
Member

bryevdv commented Mar 16, 2023

@chinmaychahar almost there, the example file still showing up in the PR due to formatting differences from the original. I've left a suggestion.

@chinmaychahar
Copy link
Contributor Author

Done, apologies for the minor errors every time! @bryevdv

@bryevdv
Copy link
Member

bryevdv commented Mar 16, 2023

No apologies necessary @chinmaychahar OSS is always an iterative process, thanks for the PR!

@bryevdv bryevdv merged commit 00fd457 into bokeh:branch-3.2 Mar 16, 2023
@chinmaychahar chinmaychahar deleted the patch-1 branch March 19, 2023 09:27
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.

None yet

3 participants