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

findBower() returns NULL #53

Closed
yutannihilation opened this issue Dec 23, 2014 · 8 comments
Closed

findBower() returns NULL #53

yutannihilation opened this issue Dec 23, 2014 · 8 comments

Comments

@yutannihilation
Copy link

I don't install Bower yet, so scaffoldWidget() should stop here:

 if (findBower() == ""){

But findBower() returns NULL, accordingly this if-statement fails.

I think else is needed here, instead of here (I'm using Ubuntu 14.04)

@timelyportfolio
Copy link
Collaborator

Interesting I will try to debug. What do you get when you

Sys.which("bower")

?

@ramnathv
Copy link
Owner

Thanks @yutannihilation for the bug report. @timelyportfolio I think a NULL check would be good to add in any case. Maybe, we need a function like this

is_bower_installed <- function(){

}

@yutannihilation
Copy link
Author

@timelyportfolio @ramnathv
Sorry for not describing the detail. I got this:

> Sys.which("bower")
bower 
   "" 

This code checks two conditions; (1) if bower exists in PATH and (2) if the OS is Windows. But the problem is that this doesn't cover the case where Sys.which("bower") is "" and .Platform$OS.type is NOT "windows". This is the reason why I got NULL.

  if(Sys.which("bower") == "") {
    if(identical(.Platform$OS.type,"windows")) {
      Sys.which(file.path(Sys.getenv("APPDATA"),"npm","bower."))
    }
  } else {
    Sys.which("bower")
  }

To fix this, you can easily add one more else like this:

  if(Sys.which("bower") == "") {
    if(identical(.Platform$OS.type,"windows")) {
      Sys.which(file.path(Sys.getenv("APPDATA"),"npm","bower."))
    } else {
      Sys.which("bower")
    }
  } else {
    Sys.which("bower")
  }

Generally speaking, nested if-statement should be avoided if possible. I prefer simpler one like this:

  bowerPath = Sys.which("bower")

  if(bowerPath == "" && identical(.Platform$OS.type,"windows")) {
      bowerPath = Sys.which(file.path(Sys.getenv("APPDATA"),"npm","bower."))
  }

  return(bowerPath)

But, I totally agree with @ramnathv's idea of is_bower_installed(). Perfect!

@timelyportfolio
Copy link
Collaborator

Understand completely. Was confused because I fixed once in an earlier version but somehow I did not apply the fix to my pulled branch. Will try again and make sure I do properly this time.

Thanks so much.

timelyportfolio added a commit to timelyportfolio/htmlwidgets that referenced this issue Dec 24, 2014
@yutannihilation
Copy link
Author

Oh, thank you for using my code. It's my honor :)

@timelyportfolio
Copy link
Collaborator

Thanks for the detailed bug report and code. Now let's see some widgets :)

@timelyportfolio
Copy link
Collaborator

@yutannihilation, can we close this? Thanks so much for all the widgets.

@yutannihilation
Copy link
Author

@timelyportfolio, Yes! Now I close this issue. Thanks again for fixing this!

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

No branches or pull requests

3 participants