dapetcu21 / atom-autocomplete-lua

Extensible autocomplete+ provider for Lua
35 stars 6 forks source link

Improve autocompletion suggestions using the "named types" scope #8

Closed GHGibonus closed 7 years ago

GHGibonus commented 7 years ago

I found autocomplete-lua to be somewhat limited, especially when it comes to suggesting members for a variable which type is not specifically stated in the code. Indeed, the best it can do is suggest members already accessed before! This is very limiting and proves to be a great weakness.

I added a functionality, enabled in the settings menu using the Table member suggestion strategy option (by default disabled). Once activated, autocomplete-lua can use the namedTypes defined in the .luacompleterc (and anywhere else) to judge if a variable is of any type defined there when the members of an unknown type variable is accessed, and suggest all the members of that type (instead of just suggesting the ones explicitly stated in the code).

The caveats are as following:

I'll also add that I had no clue how your code worked (very little documentation and code commenting) and I might have just broken your design and architecture patterns. This is up to you to chose how to integrate the changes. Due to that, I also did some very minor refactoring left and right, this was part of the learning process of understanding the flow of your program.

This is the first time I'm doing a pull request or contributing to an open project, so I'm sorry if I'm breaking any conventions!

Note that I removed that line at options/index.js:44 I'm not sure why it was there, but I needed it to get my code working, tell me if it breaks something!

dapetcu21 commented 7 years ago

Hi. Thanks for the PR. I'll thoroughly go through it at the end of the week, as right now I'm in the middle of final exams and my focus is limited.

Also, thanks for the comments. Yes, the code isn't very well documented and some parts do need a refactor. I was somewhat in a rush when I wrote this plugin.

GHGibonus commented 7 years ago

Please consider reviewing the changes and telling me whether you can integrate them or not. I have a package that heavily relies on your luacompleterc auto-completion provider, and my change makes it just so much more powerful.

If you can't integrate the pull request, tell me right now. This way I can fork your package and use my branch as dependency, so my users can benefit from my changes directly.

dapetcu21 commented 7 years ago

Hi. Don't worry. I haven't forgot about this. It just happens that I had a very busy schedule, as it's the end of the semester and final exams prevented me from focusing on anything else. I'll try to deal with this today/tomorrow.

dapetcu21 commented 7 years ago

Hi. I've been looking over the pull request for the past hour or so and here's a few notes. Sorry it took me this long to answer, but, as stated above, my time really didn't allow it in this specific period.

So here are the notes:

The takeaway from this: For the future, when submitting PR's try to make them short and only deal with one feature at a time. This makes them so much easer to review and debug. Also, if you need a feature and it's not trivial to implement, or requires a significant amount of refactoring, make an issue and start a discussion about what you want to do. The project maintainer can help you understand the code and you can make decisions about the implementation together.

As the code's not working and it's really hard for me to debug it, I can't accept your PR, but, as I was planning to do a revamp (and code cleanup) of the type system anyway, I'll add some heuristics to that, as I fully agree with your case for better heuristics. I plan on doing that pretty soon (this week or so, now that I'm free from exams), so there should not be a need for a fork.

My takeaway from this: I'll document my code and architecture a bit better and I'll add some tests, since this project started to take off quite a bit and PRs start popping up.

Now, let me see if I fully understood your implementation: Tell me if I got something wrong.

CompletionStrategy.TYPE_ASSUMPTION picks a type from namedTypes that looks similar to the type of the current scope/table. I didn't fully understand your heuristic. Why pick the type with the lowest number of fields? I'd rather pick a type that contains almost all the fields the previous type contains (+- some tolerance), not just comparing their cardinality.

CompletionStrategy.ALREADY_USED_ONLY is equivalent to my suggestUsedMembers option.

GHGibonus commented 7 years ago

I did this pull request because I felt a bit too shy to "request" a feature and choose to implement it myself to not be too demanding. I'll take note that I should consider submitting issues first and then eventually work with the maintainer on a pull request in the future.

This was also the first time I used Javascript and that might explain some of the oddities in my code. My workflow consisted mostly of putting some debugger statement left and right to see the content of the Object that were being passed around and modify the code a bit to see if I understood what I was seeing.

To answer your question: I was typing out a five paragraph explanation of my algorithm, but as I wrote it, I figured it was actually pretty flawed. My logic behind it was that in the case of an OOP API (which was what I was dealing with), to pickup the type which has the least amount of methods, but does still have all the accessed methods, because it is the most general, and the one you are probably using. But as soon as there is several types with methods named similarly outside of inheritance (which is something positive), or that there is several types with the same amount of fields, my heuristic becomes very flawed.

The best was to handle the type inference is indeed to make a list of all the accessed methods and logically work your way out of that. I was limiting myself for no real reasons to only counting the amount of methods in the previous inferred types...

The ALREADY_USED_ONLY bit of code is indeed your code, as I wanted a way to revert my implementation, as I expected it to be an annoyance when the user defines his own types. And I think it can still be an issue, even with a valid implementation.

Thank you for taking the time to review my code, and especially to respond nicely. I learned a lot from this too.