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

Use 'if' to create completions #27

Merged
merged 6 commits into from Oct 29, 2018

Conversation

Projects
None yet
4 participants
@Levertion
Contributor

Levertion commented Sep 5, 2018

I'm not certain that this implementation is correct for the codebase, but it doesn't break any of the other tests (as added in #24) and passes my new test. @Fer0x: if you could validate that this looks right too.

I've only added one test case, but want to check that this feature is desired before adding more tests.

@msftclas

This comment has been minimized.

msftclas commented Sep 5, 2018

CLA assistant check
All CLA requirements met.

@aeschli

This comment has been minimized.

Contributor

aeschli commented Sep 6, 2018

Cool!
What do you think about the following:
if if already matches, we could also suggest the then schema, otherwise the else and the if

@Levertion

This comment has been minimized.

Contributor

Levertion commented Sep 6, 2018

I think that should already be what is happening, although I haven't validated it. I was more worried that I was breaking some invariants of SchemaCollector in my code, as there is not very much documentation

@Fer0x

This comment has been minimized.

Fer0x commented Sep 16, 2018

@Levertion works correct for me. Thanks for enhancement.

Levertion and others added some commits Oct 24, 2018

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 24, 2018

@Levertion The test is still failing...

@Levertion

This comment has been minimized.

Contributor

Levertion commented Oct 24, 2018

Sorry, I've been busy with other stuff. My latest commit was to clear my working tree before I opened #30.
I should now have the time to look into finishing these tests, so I'll do that now.

@Levertion

This comment has been minimized.

Contributor

Levertion commented Oct 24, 2018

OK - I've written enough tests to convince myself that the behaviour is correct, although I'm unsure about the formatting.

Should we format automatically somehow - clearly the style is to use ', but my personal habit is to use " so that is what I have used. I would expect there to be automatic formatting, but the builtin vscode formatting changes the style of the rest of the file, prettier is not being used and the tslint rules used does not control formatting.

My personal preference is prettier, but if the current formatting needs manually editing, let me know.

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 24, 2018

just use the built-in TypeScript formatter with default settings

@Levertion

This comment has been minimized.

Contributor

Levertion commented Oct 24, 2018

I have tried that, using both format document and format selection after commenting out my entire settings.json and disabling all extensions for the workspace. This however led to only the minimal diff seen in 9f3ac83, which doesn't seem optimal. Is that enough?

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 24, 2018

Yes, that's fine. The formatter preserves new lines, that's why the changes are so small.

@Levertion

This comment has been minimized.

Contributor

Levertion commented Oct 25, 2018

Well in that case I think this PR is finished, unless there are any other changes you want me to make

@aeschli aeschli merged commit 15e5a63 into Microsoft:master Oct 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 29, 2018

It's great, thanks a lot @Levertion !

@aeschli aeschli added this to the October 2018 milestone Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment