8000 Config plot option by aneda · Pull Request #260 · plotly/plotly.py · GitHub
[go: up one dir, main page]

Skip to content

Config plot option #260

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 21 commits into from
Jul 20, 2015
Merged

Config plot option #260

merged 21 commits into from
Jul 20, 2015

Conversation

aneda
Copy link
Contributor
@aneda aneda commented Jul 15, 2015

@theengineear @chriddyp @cldougl
This is a draft for config_plot_option (world_readable). I haven't tested for python 3 yet but I wanted check if I am on the right track

from plotly import session


class FileToolsTest(TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

\o\ |o| /o/

Otherwise, it’s pretty easy to munge them if tests change them!
About to add session stashing, so it’s helpful to specify which
cre
8000
dentials/config we’re stashing/restoring.
@theengineear
Copy link
Contributor

@aneda you're tests are failing because you have an old version of master.

You should handle it in this order:

  • I'd accept the merge for my branch into yours in GH first (go ahead and click the merge button there is you're OK with it, or drop me a note if you're not, it's just a merge into your branch, not master.)
  • Then refresh your local branch git pull origin config_plot_option with your branch checked out (which is likely the state it's in)
  • Then merge the newest master into your branch (likely a small merge conflict) git pull origin master with your branch checked out (again, that's probably how it is)
  • Fix the merge conflict, commit and push back up.

Apologies if those instructions are overly detailed. I just want to make sure you're good to go! Also, let me know if you've got any questions 😸

@theengineear
Copy link
Contributor

@aneda what do you think about #260 (comment)

I apologize for possibly steering you in a slightly wrong direction, but I'd like this to be as simple to implement as possible. Seeing the code so far, I'm realizing that the nested plot options might lead to more complication than it's worth.

I'm really okay either way, as long as the file remains fairly stupid-proof.

@@ -50,7 +50,7 @@
DEFAULT_PLOT_OPTIONS = {
'filename': "plot from API",
'fileopt': "new",
'world_readable': True,
'world_readable': False,
Copy link
Member

Choose a reason for hiding this comment

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

@theengineear - My thought is to include saving the world_readable as a "required" step in the new getting-started instructions and not changing the default behaviour. So, everyone who is new will save the default to False, and be aware of privacy, and we don't introduce the backwards incompatibility for everyone else who is upgrading.

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that sounds better to me! To be clear then, @aneda , let's change this back to 'world_readable': True to maintain backwards compat.

@chriddyp
Copy link
Member

@theengineear - yeah, changeling through github releases. still seems like the best way, since it includes the exact commit

aneda and others added 6 commits July 16, 2015 12:37
```
======================================================================
ERROR: Failure: TypeError (__init__() takes exactly 1 argument (2
given))
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/usr/local/lib/python2.7/site-packages/nose/loader.py", line
523, in makeTest
   return self._makeTest(obj, parent)
 File "/usr/local/lib/python2.7/site-packages/nose/loader.py", line
570, in _makeTest
   return self.loadTestsFromTestCase(obj)
 File "/usr/local/lib/python2.7/site-packages/nose/loader.py", line
494, in loadTestsFromTestCase
   return super(TestLoader, self).loadTestsFromTestCase(testCaseClass)
 File
"/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.
7/lib/python2.7/unittest/loader.py", line 56, in loadTestsFromTestCase
   loaded_suite = self.suiteClass(map(testCaseClass, testCaseNames))
TypeError: __init__() takes exactly 1 argument (2 given)
```
@aneda
Copy link
Contributor Author
aneda commented Jul 17, 2015

@theengineear @chriddyp @cldougl
I changed the removed the plot_option with world_readable = True. Please let me know what needs to be modified/ add.
Btw, thanks for all the reviews and help. Andrew I don't think it is ever overly detailed for me haha

@theengineear
Copy link
Contributor

@aneda this is looking good. if you want me to pull this down and get tests running again let me know. they're failing because of a change @BRONSOLO recently made that we're being a bit too strict about in the api library, imo.

https://github.com/plotly/python-api/blob/config_plot_option/plotly/tests/test_core/test_grid/test_grid.py#L194

That file needs to be updated to use a TestCase and it should not depend on the 409, but rather a 4xx. Else, you can skip that test, I'm open to either.

@aneda
Copy link
Contributor Author
aneda commented Jul 17, 2015

@theengineear did I do it right?


Returns all if no arguments are specified.

Example:
get_credentials_file('username')
get_credentials_file('plotly_domain')
Copy link
Contributor

Choose a reason for hiding this comment

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

get_config_file?

@theengineear
Copy link
Contributor

Looking good! One blocking comment about the _args to *_kwargs change.

@aneda
Copy link
Contributor Author
aneda commented Jul 17, 2015

@theengineear is this good? Also would let me know if the issue does make sense or not, thanks!

@aneda
Copy link
Contributor Author
aneda commented Jul 17, 2015

@theengineear just updated the kwargs to args

@theengineear
Copy link
Contributor

hmmm, not sure why this new test is failing? i'll take a closer look tomorrow.

@chriddyp do you think breaking a small amount of backwards compat is OK here:

#260 (comment)

@aneda
Copy link
Contributor Author
aneda commented Jul 17, 2015

@theengineear I added another @skip as Chris suggested! Is it good now!

cc @chriddyp

@theengineear
Copy link
Contributor

💃 !

@theengineear
Copy link
Contributor

If we're going to release on pypi, we also need a version bump!

@aneda
Copy link
Contributor Author
aneda commented Jul 20, 2015

@theengineear @chriddyp is it good to go?

@theengineear
Copy link
Contributor

si! 👯

@chriddyp
Copy link
Member

merge away!

On Mon, Jul 20, 2015 at 2:37 PM, Andrew notifications@github.com wrote:

si! [image: 👯]


Reply to this email directly or view it on GitHub
#260 (comment).

Chris Parmer
Plotly, Chief Product Officer
514 571 5897
https://plot.ly/team

aneda pushed a commit that referenced this pull request Jul 20, 2015
Config plot option: world_readable
@aneda aneda merged commit 993b30d into master Jul 20, 2015
@aneda aneda deleted the config_plot_option branch July 20, 2015 21:21
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.

3 participants
0