Skip to content

fix config settings and add the rest of plot options to config #303

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 45 commits into from
Sep 30, 2015

Conversation

chriddyp
Copy link
Member

hey @theengineear @cldougl can you take a look at this? I found the previous plot_option code super confusing to navigate, so I rewrote a bunch of it. Also, I added a ton of tests.

this fixes #302

- why is get_plot_options in the root namespace? its name is so
misleading - _plot_option_logic is the function that actually sets the
options
- make the precedence *extremely* clear.
- make the world_readable + sharing dependency explicit
- update the variable names: what was "current"? what was
"plot_options". it's now `user_plot_options` - stuff that the user
supplied and `plot_options_from_call_signature` - stuff that pass
through from iplot, plot
we're now writing the default arguments to the disk. But we're also
using `DEFAULT_PLOT_OPTIONS` in the namespace for updating
`plot_options`.
now that the `_plot_option_option` logic is nice n generic, let's add
some more options.
@theengineear
Copy link
Contributor

@chriddyp cool, got some comments for ya. I think it's good that you're looking into this refactor. It's definitely confusing and got much more so when we added some of the 'sharing' stuff in a recent pr.

Another thing that would be nice... i think we need to reorganize the python api files. here are a couple things that jump out:

  • tools.py really shouldn't depend on graph_objs, which means we should move graph_obj-related functionality info the /graph_objs package (e.g., /graph_objs/figure_factory.py)
  • getting import info for pandas and such should not require importing tools
  • related to above, a circular-import-proof way to know which optional deps are imported would be great
  • revisit the /plotly/plotly package in terms of keeping it restricted to network-communication logic to plot.ly

- why is get_plot_options in the root namespace? its name is so
misleading - _plot_option_logic is the function that actually sets the
options
- make the precedence *extremely* clear.
- make the world_readable + sharing dependency explicit
- update the variable names: what was "current"? what was
"plot_options". it's now `user_plot_options` - stuff that the user
supplied and `plot_options_from_call_signature` - stuff that pass
through from iplot, plot
we're now writing the default arguments to the disk. But we're also
using `DEFAULT_PLOT_OPTIONS` in the namespace for updating
`plot_options`.
now that the `_plot_option_option` logic is nice n generic, let's add
some more options.
@chriddyp
Copy link
Member Author

alright @theengineear - this one's for you: 4d367ab

good to 💃 ?

@chriddyp
Copy link
Member Author

alright @theengineear - tests are fixed.

- You can also save `auto_open` and `sharing` to the config file so that you can forget these
keyword argument in `py.iplot` and `py.plot`.


Copy link
Contributor

Choose a reason for hiding this comment

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

oops, after praising the changelog, i totally didn't add a note! gah!

Also, this should now be 1.8.6, the update hit at 1.8.5, mind giving it a quick change?

@theengineear
Copy link
Contributor

@chriddyp looks good!

  • Please consider the 🐄s I mentioned
  • Also, just a quick update on the version number and the changelog version number

Other than that 💃 :)

chriddyp added a commit that referenced this pull request Sep 30, 2015
fix config settings and add the rest of plot options to config
@chriddyp chriddyp merged commit 0880002 into master Sep 30, 2015
@chriddyp chriddyp deleted the fix-config branch September 30, 2015 21:26
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.

world_readable config is broken
2 participants