Skip to content

check types with isinstance; allow tuple, where possible #770

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

Merged
merged 2 commits into from
Nov 30, 2020

Conversation

dizcza
Copy link
Contributor

@dizcza dizcza commented Nov 22, 2020

Dear Jack,

I stumbled upon a bad practice of checking the object type with type(x) == y. Also, a handy-used scenario is when the user populates a dict with keys as legend labels and values as the payload:

names, values = list(zip(*dict.items()))
viz.line(..., opts=dict(legend=names))

In short, legend sometimes can be passed as a tuple rather than a list, and I thought there is nothing wrong with allowing tuples.

How Has This Been Tested?

Tested by running the demo script.

Types of changes

  • Other (non-breaking change which improves code readability)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • For JavaScript changes, I have re-generated the minified JavaScript code.

P.S. It ain't much, but it's honest work.
P.P.S. When I was fixing the bad practice here and there, I noticed some other small issues with the coding style like code duplication, old python usage, etc. At first, I felt tempted to dive deeply into fixing them all but noticed #675, which, of course, should be merged before any further sugar-code fixes.
P.P.P.S. One could set up Travis or CircleCI builds like I'm doing here to finally test the changes properly on each PR. I can write CircleCI build script if you set up a server.

Best,
Danylo

Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

Style fixes are always appreciated! Definitely hoping to get #675 in sometime in the very near future as we prepare transitioning the project. And following that I'm hoping standardizing some CI for Visdom will be next on the docket. Happy to tag you with an up-for-grabs when we get CI set up.

As always, thanks for the contribution!

@JackUrb JackUrb merged commit 8200327 into fossasia:master Nov 30, 2020
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.

3 participants