-
Notifications
You must be signed in to change notification settings - Fork 24
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
Better compatibility to Android IDE local packages installation #12
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Sorry for the slow response, I've been off the grid. I think this is a great addition, but I have a couple requests and some questions before merging.
endif | ||
endfor | ||
endfor | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you take the shared logic here and factor it out into a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
endif | ||
endfor | ||
endfor | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also looks like a lot of shared logic that could be factored out into a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
endif | ||
return arduino_dir | ||
endfunction | ||
|
||
function! arduino#GetArduinoUserInstallation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you actually need this function. Since you do the exists()
check at the top of the file, any time you're calling this function you can instead just read out the value of g:arduino_user_installation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user should be able to configure the directory, because it can be configured in the arduino ide as well.
So I think I'll need this check to verify the existence of the directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm misunderstanding, but at the top of the file you're doing
if !exists('g:arduino_user_installation')
if s:FileExists(s:ARDUINO_USER_DIR)
let g:arduino_user_installation = 1
else
let g:arduino_user_installation = 0
endif
endif
So by the time this method is called, the only way exists('g:arduino_user_installation')
will be false is if the user has manually run unlet g:arduino_user_installation
somewhere. It seems like you could just access g:arduino_user_installation
directly instead of calling this getter.
autoload/arduino.vim
Outdated
call add(boards, package . ':' . arch . ':' . board) | ||
endif | ||
if arduino#GetArduinoUserInstallation() == 1 | ||
for filename in split(globpath(arduino_dir . '/packages', '**/boards.txt'), '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the difference in logic here (searching /packages/
instead of /hardware
, pulling out the package version, etc) due to the user installation? I would expect that boards might exist in the /packages
dir even if system installed, and that boards would exist under /hardware
in a user installation. Is this the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you got the point correct: I realized with your plugin, that all my hardware, etc. (installed by the arduino ide into the user dir) wasn't available.
That's why I started to dig into the code.
autoload/arduino.vim
Outdated
call add(programmers, package . ':' . programmer) | ||
endif | ||
if arduino#GetArduinoUserInstallation() == 1 | ||
for filename in split(globpath(arduino_dir . '/packages', '**/programmers.txt'), '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about if this is specific to a user installation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is.
autoload/arduino.vim
Outdated
@@ -54,6 +54,9 @@ function! arduino#InitializeConfig() | |||
if !exists('g:arduino_run_headless') | |||
let g:arduino_run_headless = executable('Xvfb') ? 1 : 0 | |||
endif | |||
if !exists('g:arduino_user_installation') | |||
let g:arduino_user_installation = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, you could default this to 1 if s:FileExists($HOME . ".arduino15")
. If you do, you should probably pull $HOME . ".arduino15"
out into a constant or a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
If you're using a non default path you also have to set 'g:arduino_dir'. > | ||
let g:arduino_user_installation = 0 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll want a closing <
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Hi Thanks for the feedback - will check it out and update the PR :) |
Some Information / Questions: With my actual changes the user can only choose between the system installation or the user installation. Both can't be used simultaneously I would go with the g:arduino_user_installation variable and would add an additional g:arduino_user_dir which will point to the user path (as already started in my code). The additional stuff would be to only extend your logic (boards, programmers, etc.) with the values found in the user dir, when the g:arduino_user_installation variable is set. What do you think about this? BR, |
Yeah, I would definitely be in favor of searching both the user installation and the system installation. I'd recommend making |
Hi,
I've create a patch to be able to use my ~/.arduino15 installation directory from the arduino ide.
Without that I wasn't able to retrieve the boards I've installed with my normal user and the actual ide 1.8.x into my user home folder.
I hope you're going to include that into the mainstream :)