Update vectorbenderdialog.py

olivierdalang / VectorBender

VectorBender is a QGis plugin

GNU General Public License v3.0

37 stars 15 forks source link

Update vectorbenderdialog.py #28

Closed busterbeam closed 3 years ago

busterbeam commented 3 years ago

Modified script without running any tests, improved variable naming, and utilized loops for repetitive/sequential code. add line spaces between methods for better readability.

Notability improved createMemoryLayer by changing suffix to integer which is incremented in while loop without condition. Then utilized f literal strings for spacing in output. Changed any formatted strings to f literals. Lastly modified checkRequirements so uses if/elif/else logic with needing return in each conditional snippet

olivierdalang commented 3 years ago

Closing as this looks suspicious to me, if it's a legit PR please explain what you're trying to achieve and group all related changes under one PR

busterbeam commented 3 years ago

Closing as this looks suspicious to me, if it's a legit PR please explain what you're trying to achieve and group all related changes under one PR

sorry it looks suspicious, I was planning to test my changes, just wondered if your policy was more lax or you would run your own tests anyway before pulling anyway. Also did them separately as I probably wouldn't remember all the discrete changes. I mainly try improve readability and extensibility and performance where possible. I wasn't adding or removing functionality, you could say I added functionality by separating larger chunks of code into discrete methods.

I will come back with a better pull when I find out what my friend is having a problem with in the plug-in. Apparently this package is buggy.

olivierdalang commented 3 years ago

The plugin is indeed buggy as I don't have time to maintain it, so improvements are welcome.

But please do so in well organized PRs :

1 PR for code style (for that, I'm open to using black with a pre-commit config, but would like to avoid having a large manual update of code style). Please do not include any other change besides code formatting in that PR.
1 PR for refactoring (but unsure it's worth working on this per se, as it's highly a question of personal preference). You can improve/refactor parts while working on them.
N PRs for fixing issues / performance improvements, ideally with associated issue before working on the PR itself.

This way it's possible for me to properly review.

Also unfortunately there's no automated tests yet, so that would also be a great addition. In the mean time, you'll have to tests the PRs manually.

busterbeam commented 3 years ago

Alright thank you for this, I will get back to you on this, but a few more questions.

code styling, black black with a pre-commit config? are you saying you have a specific config in mind or if I edit the config of black submit it in my commit?
(not a question) I will refactor for readability and extensibility/flexibility... anything that can reduce headaches down the road.
buggy? I will admit to never using Qgis and I'm mainly going to improve this for a friend who uses it, he has yet to submit to me or here as an issue the exact "bugs". If you could provide some precise logs or pointers to what problems your having. I will try a play around with the plug-in when I get the chance and say where it's "buggy".

I might make some tests once I understand the plug-in a little better, on that note though some of the modules use assert statements which are actually debugging/testing keywords, not proper form