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

Reformat docstrings #1835

Merged
merged 12 commits into from
Oct 22, 2019
Merged

Conversation

joelostblom
Copy link
Contributor

@nicolaskruchten As per our discussion in plotly/plotly_express#154

I tried not making more changes than necessary so I didn't go after things like changing "numbers" to "int" or "float". I used make_subplots.py as a guide and tried to be consistent with the formatting there (with the one exception being dict that I changed there instead according to what seemed to be the most frequently used elsewhere in plotly).

Please have a thorough look through since I might have missed some parenthesis somewhere.

Close plotly/plotly_express#154

@joelostblom
Copy link
Contributor Author

I'm note sure how to fix the Python2 related test errors, I believe unpacking operators have been available in Python for quote some time, so not sure why *colref is marked as invalid syntax.

@emmanuelle
Copy link
Contributor

Thank you for your pull request, it is clearly an improvement, also in plain ipython :-). Yeah, it looks like *colref is not valide py2 syntax. You have several options to fix this I think, either use list comprehension, or colref[0] and colref[1]. I'll merge once the CI is fixed. Thanks again!

@joelostblom
Copy link
Contributor Author

joelostblom commented Oct 21, 2019

@emmanuelle I had to reformat one more thing for Python2 compatability and now all tests are passing!

],
dimensions=[
"(list of strings, names of columns in `data_frame`)",
"Columns to be used in multidimensional visualization.",
"list of str",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a colref_list_type no? @emmanuelle thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, correct, we forgot to update this one. @joelostblom would you mind making this change? Otherwise I can push a commit to your branch if you don't have the time. And then we'll merge :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emmanuelle Updated!

@nicolaskruchten
Copy link
Contributor

This is pretty great, thank you! Modulo my one comment we can merge this :)

@joelostblom
Copy link
Contributor Author

The failed test seems unrelated.

@nicolaskruchten
Copy link
Contributor

This is really nice work, thanks very much! 💃

@nicolaskruchten nicolaskruchten merged commit be1a182 into plotly:master Oct 22, 2019
@joelostblom joelostblom deleted the reformat-docstring branch October 22, 2019 04:56
@joelostblom joelostblom mentioned this pull request Nov 4, 2019
@nicolaskruchten nicolaskruchten mentioned this pull request Nov 12, 2019
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.

Format docstrings to facilitate reading
3 participants