NPE fix, Support keywords from nested classes, Simplify pyFunction search

AmailP / robot-plugin

Robot Framework support plugin for IntelliJ Idea

Other

72 stars 38 forks source link

NPE fix, Support keywords from nested classes, Simplify pyFunction search #80

Closed skhomuti closed 4 years ago

skhomuti commented 4 years ago

Hello! I'm trying to fix the problem of resolving methods of nested classes

skhomuti commented 4 years ago

from robotframework==3.1.*

Importing test library 'module_a' failed: ModuleNotFoundError: No module named 'module_a'

AmailP commented 4 years ago

Hi @skhomuti, thank you very much for your contribution. It is greatly appreciated.

In order to merge your pull request I would ask you to provide one or more tests, so:

it will make clear what is currently not working (the test/s should fail with the current code)
it will prevent later to have regressions

Thanks again, Valerio

skhomuti commented 4 years ago

@AmailP Sure! Added a test. Do you think you need to add something else? Scala is a new language for me, comments on the code style will also be useful to me

AmailP commented 4 years ago

@skhomuti Thank you very much again.

Since you are asking for more feedback from me I would let you choose how to proceed with this merge request.

Usually when I get a contribution I am not too picky with the committer, so what I do is merging the commit and successfully refactor/adapt the code to my way of working/thinking.

If you are willing to dig deeper, we can start a review process like a would do with a project member. This would be a longer process but it will probably be probably more instructive for the both of us.

Please let me know how do you want to proceed.

skhomuti commented 4 years ago

Let's go the longer way. It will be a great experience for me.

skhomuti commented 4 years ago

@AmailP please see the last commit. After the review, I will squash commits

skhomuti commented 4 years ago

The proposed option falls with NPE. I decided to try to get rid of Optional and made a collection instead of a single value. The result was almost what I would like to see it.

I apologize for the improvements now, it seems I should have done it right away.

AmailP commented 4 years ago

@skhomuti It looks good to me now :-)

Final refinements:

rebase on top of the new master
[optional] I would squash commit 2 and 3 since they are an evolution inside a single feature branch
[optional] I tend recently to stick to this article for commit messages (expecially for rule 5): https://chris.beams.io/posts/git-commit/

Great job! And thank you for your patience.

skhomuti commented 4 years ago

@AmailP Interesting article, thanks!

I am grateful for so much time devoted to reviewing my code. There is always something to learn. :)