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

adding repo and branch options to updateplotlyjsdev #2349

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

zouhairm
Copy link
Contributor

@zouhairm zouhairm commented Apr 2, 2020

PR to address #2344

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).

@zouhairm zouhairm force-pushed the dev/issue#2344 branch 2 times, most recently from ac834bb to 9c27404 Compare April 2, 2020 21:39
@zouhairm
Copy link
Contributor Author

zouhairm commented Apr 2, 2020

looks like circle CI for 2.7 is failing (seems to be due to something unrelated to this PR though)

response = <Response [400]>

    def validate_response(response):
        """
        Raise a helpful PlotlyRequestError for failed requests.
    
        :param (requests.Response) response: A Response object from an api request.
        :raises: (PlotlyRequestError) If the request failed for any reason.
        :returns: (None)
    
        """
        if response.ok:
            return
    
        content = response.content
        status_code = response.status_code
        try:
            parsed_content = response.json()
        except ValueError:
            message = content if content else "No Content"
            raise exceptions.PlotlyRequestError(message, status_code, content)
    
        message = ""
        if isinstance(parsed_content, dict):
            errors = parsed_content.get("errors", [])
            messages = [error.get("message") for error in errors]
            message = "\n".join([msg for msg in messages if msg])
        if not message:
            message = content if content else "No Content"
    
>       raise exceptions.PlotlyRequestError(message, status_code, content)
E       PlotlyRequestError: Sorry, a file named 'is_share_key_included2_grid' already exists

@emmanuelle
Copy link
Contributor

Thank you very much for your PR @zouhairm ! The CI failure is indeed unrelated to your PR has just been fixed in master. You can merge master and push to be sure that everything is ok.

@emmanuelle
Copy link
Contributor

So this is really a cool pull request. I just used your branch to build with a development branch currently developed in plotly.js, this is going to be so useful. The only thing I want to check before merging is that the PR modifies the name of the plotlyjs version, even in the case of plotly/master. I wonder if we have things in the repo depending on this name. All tests pass which is a good baseline. @jonmmease is it ok for you modifying the plotlyjs version name? It's also easy to slightly modify the PR so that it is not changed in the case plotly/master.

@zouhairm
Copy link
Contributor Author

zouhairm commented Apr 5, 2020

Sounds good. Feel free to make the change or lmk and I can do it.

@nicolaskruchten
Copy link
Contributor

This is a great change, thanks so much!

@nicolaskruchten nicolaskruchten merged commit 141aa6d into plotly:master Apr 6, 2020
@zouhairm zouhairm deleted the dev/issue#2344 branch April 7, 2020 00:48
@nicolaskruchten nicolaskruchten added this to the 4.7.0 milestone Apr 27, 2020
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.

None yet

3 participants