Skip to content

use correct non-vectorized version of logical and #250

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 1 commit into from
May 27, 2021
Merged

use correct non-vectorized version of logical and #250

merged 1 commit into from
May 27, 2021

Conversation

daattali
Copy link
Contributor

No description provided.

# ensure that assets_folder is neither NULL nor character(0)
if (!(is.null(private$assets_folder)) & length(private$assets_folder) != 0) {
if (!(is.null(private$assets_folder)) && length(private$assets_folder) != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this change have any observable consequences (which we should lock down with a test), or is it just fixing poor practice that nonetheless happens to work?

Copy link
Contributor Author

@daattali daattali May 26, 2021

Choose a reason for hiding this comment

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

As long as private$assets_folder is a single value rather than a vector/list, there should be no behavioural change. From my understanding, assets_folder should be a single value, but I don't see any checks for it so I'm not sure what currently happens when you pass in a vector. I may have been too hasty in this PR because I don't actually know for sure that assets_folder needs to be a single string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, should be a single string. FWIW here's the docstring for the corresponding param in Python

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

@alexcjohnson alexcjohnson merged commit 4eac3ea into plotly:master May 27, 2021
@alexcjohnson
Copy link
Collaborator

Ah shoot, didn't notice the base branch here is master rather than dev. Going to revert it so we don't have conflicts later on, but it's a good find that we should carry over to dev in a separate PR. @daattali please note there's a good deal of work in the dev branch that has yet to be released, as you're checking out the code. Most importantly #243

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.

2 participants