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

Allow user to customize items #2847

Merged
merged 6 commits into from
Nov 15, 2019

Conversation

DonnieWest
Copy link
Contributor

Continues from #2710

Allows the user to set their own completion fields

Relies on #2845

@DonnieWest
Copy link
Contributor Author

@w0rp looks like it's failing on vint although the syntax is correct. Any advice for making it pass?

@w0rp
Copy link
Member

w0rp commented Oct 18, 2019

The error message printed says there's no scriptencoding, so add one. It's complaining because there a Unicode characters in the file without declaring the script as using UTF-8.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need some tests and a few changes.

@@ -36,23 +36,23 @@ Execute(TypeScript completion details responses should be parsed correctly):
\ 'word': 'abc',
\ 'menu': '(property) Foo.abc: number',
\ 'info': '',
\ 'kind': 'f',
\ 'kind': 'v',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests should still pass, as we should keep the existing defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to maintain strict backward compatibility?

Seems to me we'd want to make these roughly match the kinds that vim has built in:

	v	variable
	f	function or method
	m	member of a struct or class
	t	typedef
	d	#define or macro

In particular, since this is a property it would probably map best to m (not v as it is right now, sorry). Same as the Rust types above. They probably map best to t, right?

I'm happy to maintain compat though or try this new logical mapping

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@w0rp sorry to bug you, but still waiting for a definitive response here :)

test/completion/test_lsp_completion_parsing.vader Outdated Show resolved Hide resolved
@DonnieWest
Copy link
Contributor Author

Sorry for the noise. Still having trouble getting vint to pass on both this PR and #2849

I'm not sure what I'm missing here :/

@DonnieWest
Copy link
Contributor Author

@w0rp got the linter passing :)

Once I get feedback on the completion items, I should be good to go

@DonnieWest DonnieWest marked this pull request as ready for review October 30, 2019 01:32
@DonnieWest
Copy link
Contributor Author

AppVeyor issue appears to be caused by html-beautifier as noted in the other PR

@DonnieWest
Copy link
Contributor Author

@w0rp looks like the html-beautifier fix also fixed things over here. Just waiting for feedback on whether we want to actually retain backwards compat or if we're fine changing some types returned by default during completion.

Thanks :)

@DonnieWest DonnieWest requested a review from w0rp November 14, 2019 17:54
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good as it is. Let's go with this.

@w0rp w0rp merged commit b91d82b into dense-analysis:master Nov 15, 2019
@w0rp
Copy link
Member

w0rp commented Nov 15, 2019

Cheers! 🍻

@DonnieWest
Copy link
Contributor Author

Thanks!

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