Skip to content
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

Add findDocumentSymbols #7

Merged
merged 1 commit into from Feb 21, 2017
Merged

Conversation

hoovercj
Copy link
Member

@hoovercj hoovercj commented Feb 12, 2017

A question was asked in gitter if VS Code has an equivalent of WebStorm's "File Structure Popup" in HTML. The closest that vscode has is "Show document symbols", but the html language service does not provide that.

To attempt parity with webstorm in this area, I have implemented findDocumentSymbols. I have augmented the parser to track tag attributes and their values (I could see nowhere before that attributes were being used) and combine the tag name with id and class attributes to build a symbol name.

For example, <div id='test' class='my classes'></div> will have the symbol name div#id.my.classes. For child elements, I set the containerName to be the symbol name of their parent tag. So in <div id='container1'><span></span></div>, the span element would have the symbol name span and the container name div#container1

I set the SymbolKind to field just to have something as I am unsure what the convention is for non-code related symbols.

For this to appear in vscode, it would necessitate a change there as well. I have a pull request ready to submit if this is accepted.

A question was asked in gitter if VS Code has an equivalent
of WebStorm's "File Structure Popup" in HTML. The closest that vscode has
is "Show document symbols", but the html language service does not
provide that.

To attempt parity with webstorm in this area, I have implemented
findDocumentSymbols. I have augmented the parser to track tag attributes
and their values (I could see nowhere before that attributes were being
used) and combine the tag name with id and class attributes to build
a symbol name.

For example, <div id='test' class='my classes'></div>
will have the symbol name div#id.my.classes. For child elements,
I set the containerName to be the symbol name of their parent tag.
So in <div id='container1'><span></span></div>, the span element
would have the symbol name span and the container name div#container1

I set the SymbolKind to field just to have something as I am unsure
what the convention is for non-code related symbols.

For this to appear in vscode, it would necessitate a change there as well.
I have a pull request ready to submit if this is accepted.
@aeschli
Copy link
Contributor

aeschli commented Feb 21, 2017

Awesome, looks good @hoovercj !

@aeschli aeschli added this to the February 2017 milestone Feb 21, 2017
@aeschli
Copy link
Contributor

aeschli commented Feb 21, 2017

Changing attributeNames to attributes is strictly speaking an API breakage. I added attributeNames back but only in the implementation as to stay compatible.
I removed attributeNames from the d.ts given that I just went 2.0 a few days ago and I don't anticipate that many users.

@hoovercj Actually all that's missing is a full test. I'll add these.

@hoovercj
Copy link
Member Author

@aeschli what tests are missing? I tried to add tests to avoid putting extra effort on the team to accept the PR, so I'd like to know what I missed for future reference.

aeschli added a commit that referenced this pull request Feb 21, 2017
@aeschli
Copy link
Contributor

aeschli commented Feb 21, 2017

Np, see my commit above.

I'll adopt the change to VSCode so that it gets in today.

@hoovercj
Copy link
Member Author

@aeschli how is the test you added different than the one I added in src/test/documentSymbols.test.ts? Yours are organized slightly differently than mine, but they essentially cover the same thing:

    test('Simple', () => {
        testSymbolsFor('<div></div>', [<SymbolInformation>{ containerName: '', name: 'div', kind: <SymbolKind>SymbolKind.Field, location: Location.create(TEST_URI, Range.create(0, 0, 0, 11)) }]);
        testSymbolsFor('<div><input checked id="test" class="checkbox"></div>', [{ containerName: '', name: 'div', kind: <SymbolKind>SymbolKind.Field, location: Location.create(TEST_URI, Range.create(0, 0, 0, 53)) },
            { containerName: 'div', name: 'input#test.checkbox', kind: <SymbolKind>SymbolKind.Field, location: Location.create(TEST_URI, Range.create(0, 5, 0, 47)) }]);
    });

Given that you've added src/test/documentSymbols.test.ts, though, I recommend removing my src/test/symbols.test.ts

@aeschli
Copy link
Contributor

aeschli commented Feb 21, 2017

@hoovercj Sorry, I completely didn't see these tests. I must have been sleeping. Sorry.

@hoovercj
Copy link
Member Author

:-) Glad we both had similar ideas. Thanks for accepting the pull request, cheers!

@aeschli
Copy link
Contributor

aeschli commented Feb 21, 2017

I merged the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants