-
-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
issue #98 - adding initial changes #105
issue #98 - adding initial changes #105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start, but I think we're a bit off from making this work. I'll submit my own PR so you can compare.
inScenarioOutline = (scenario.examples || []).length > 0 | ||
return symbols | ||
}, | ||
step(step, symbols) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we include steps in the outline, the outline is going to look more or less like the original Gherkin document. I don't think this would be particularly helpful. I see the outline as a navigation aid, so it's got to be smaller than the original document.
I think we should exclude steps from the outline.
return walkGherkinDocument<DocumentSymbol[]>(gherkinDocument, symbols, { | ||
scenario(scenario, symbols) { | ||
inScenarioOutline = (scenario.examples || []).length > 0 | ||
return symbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The walkGherkinDocument
function works similarly to Array.reduce - the accumulator (symbols
in yor case) is passed into each callback, and the callback should return a new accumulator.
You should return a new accumulator here.
I wouldn't use DocumentSymbol[]
as an accumulator. We should build a tree, not a list.
const codeKeywords = [...dialect.given, ...dialect.when, ...dialect.then].filter(noStars) | ||
let snippetKeyword = dialect.given.filter(noStars)[0] | ||
|
||
return walkGherkinDocument<DocumentSymbol[]>(gherkinDocument, symbols, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also have callbacks for feature
, rule
and background
.
Hi @10xtechie. I'm closing this in favour of #106. I really appreciate the effort you put into this. I think it would have taken quite a bit of back and forth to explain what I had in mind, which is why I went ahead and did this myself. I think this change was a bit complicated, and difficult to pull off without good knowledge of the inner workings. Hope that's ok. |
🤔 What's changed?
Adding initial files for implementing Document Symbols Request
⚡️ What's your motivation?
Implementing Document Symbols Request
🏷️ What kind of change is this?
This is initial changes for feedback
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.