search

tomi / vscode-rf-language-server

VSCode extension to support Robot Framework development
MIT License
53 stars 35 forks source link

Support duplicate keywords with namespace resolution from Resource filenames #18

Closed edchapel closed 6 years ago

edchapel commented 6 years ago

Hello,

I ran into a similar issue as requested in #15. I've attempted an implementation to address this and tested against my personal "spec" in a sample repository at vscode-rf-language-server-robot-namespace-sample. The sample robot files include comments for what I would expect to happen based on the Robot user docs.

The changes in the PR introduce the idea of a namespace for UserKeywords and Steps. The namespace is the filename of the Resource without the extension. I added and updated tests where I've extended the keyword and test case parsing.

Unfortunately, I was not able to get the intellisense to suggest keywords from the Resources files.

Please let me know your thoughts on this PR.

Many thanks for the excellent VSCode plugin!

Ed

edchapel commented 6 years ago

In short, these snippets demonstrate what is expected:

Page1.robot

*** Keywords ***
Should Be Loaded
Page2.robot

*** Keywords ***
Should Be Loaded
Tests.robot

*** Settings ***
Resource  lib/Page1.robot
Resource  lib/Page2.robot

*** Test Cases ***
Should show intellisense for and navigate to Page1.robot
  Page1.Should Be Loaded

Should show intellisense for and navigate to Page2.robot
  Page2.Should Be Loaded
tomi commented 6 years ago

Hey, and thank you for your PR! The overall idea is good, but I think that adding a namespace property for the Identifier is a bit problematic. Identifier is used also for example for variables and test cases, which don't have a namespace concept. I think this causes issues (at least) for variables that have a dot in their name.

My suggestion is to create a new model, perhaps call it NamespacedIdentifier, that extends Identifier and adds that namespace member for it. Then parse either an Identifier or NamespacedIdentifier in places where there can be an identifier with a namespace, i.e. keyword calls (call expressions). Then I'd add that namespace also for each WorkspaceFile. Then in definition-finder in findKeywordDefinition function if the call expression uses an explicit format (i.e. has a namespace), we first find the WorkspaceFile with a matching namespace and then try to find the keyword from that file. I think modeling it this ways makes it also a bit easier to suggest only keywords from a specific keywords file if a namespace is typed first.

I think with these changes we can get this PR in.

edchapel commented 6 years ago

Hi @tomi,

I've finally managed to get the time to implement the resource file support with the NamespacedIdentifier as you suggested. I've added a couple of tests to demonstrate the explicit keyword parsing. I've also implemented changes to the completion-helper to suggest a namespace when the keyword would be ambiguous.

I did not implement any warnings or errors when the user types an ambiguous keyword without a namespace. This would be nice, but is beyond my current understanding of the VSCode language service.

Using the sample repository vscode-rf-language-server-robot-namespace-sample, the plugin behavior meets the functionality I set out to implement.

Please give me feedback if you'd like changes or if you have suggestions.

Ed

tomi commented 6 years ago

Hi @edchapel ,

Architecturally this is good now. I would have couple comments about some of the implementation details, but they are minor and I don't want them blocking this PR and I can work them out myself. Therefore I'm going to merge this as is, so we can get it released. HUGE thanks for your contribution, this is a great feature 👏 Feel free to add yourself to the list of contributors if you fancy.