search

dabrown645 / quickemu

Quickly create and run optimised Windows, macOS and Linux desktop virtual machines.
https://www.youtube.com/watch?v=AOTYWEgw0hI
MIT License
1 stars 1 forks source link

Void Linux plugin, nicer comments outside of functions, checked homepages #3

Closed zen0bit closed 8 months ago

zen0bit commented 9 months ago

Included changes:

zen0bit commented 9 months ago

Please enable Issues in repository

zen0bit commented 9 months ago

Can't find your review comments

dabrown645 commented 9 months ago

Sorry I am new to being on the receiving end. major comment was about get_files in template. In template get_files should not have any code since quickget provides. It is there to remind people that it can be overridden in the few cases where it is needed.

Also, someone pointed out the there were a bunch of spelling/typing errors I went through all the files with aspell and fixed them so looks like I impacted your request where you moved comments from inside to outside functions . Sorry

Oops. I backed the change out as I didn't so enough testing and broke it I will be redoing to correct this.

zen0bit commented 8 months ago

Sorry I am new to being on the receiving end. major comment was about get_files in template. In template get_files should not have any code since quickget provides. It is there to remind people that it can be overridden in the few cases where it is needed.

Understand but since it is template, should contain examples, and I included mentioned macos(I think) there. Maybe better mention that shouldn't be used by default,or delete that part again..

Also, someone pointed out the there were a bunch of spelling/typing errors I went through all the files with aspell and fixed them so looks like I impacted your request where you moved comments from inside to outside functions . Sorry

Oops. I backed the change out as I didn't so enough testing and broke it I will be redoing to correct this.

No problem I already repaired some spelling errors

Anything else should be changed?

dabrown645 commented 8 months ago

Understand but since it is template, should contain examples, and I included mentioned macos(I think) there. Maybe better mention that shouldn't be used by default,or delete that part again..

I guess I don't have an issue with an example but if it is not noted as an example and commented out it will always override the function in quickget. When I reviewed the original quickget it looked like 80% give or take used the same code so I wanted that in quickget but allow for it to be overridden in individual plugin If there is a change to the "standard" get_file I don't want to have to update all the plugins.

Everything else looked good

zen0bit commented 8 months ago

Done

zen0bit commented 8 months ago

🎉 And now merge it, you are a Boss 🎉