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

New: Pass content to `engine.executeOn` #1268

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@antross
Member

antross commented Aug 24, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Needed for #1254 to support validating files with unsaved changes.
Includes enhancing the local connector to use passed content.
Implemented via an options object to simplify consumption/maintenance.
New tests added for both Engine and LocalConnector to validate behavior.

antross added some commits Aug 22, 2018

Fix: Pass content directly to the local connector
Will be useful for extensions (e.g. for VSCode).
Chore: Switch to using an options object
This simplifies maintenance and consumption of future options.
Plumbed this all the way through to `fetchContent`.

@antross antross requested review from alrra, molant and sarvaje as code owners Aug 24, 2018

@antross

This comment has been minimized.

Member

antross commented Aug 24, 2018

Also, do we want to add documentation for the local connector (#1245) as part of this PR, or leave it for a separate one?

@alrra

This comment has been minimized.

Member

alrra commented Aug 24, 2018

add documentation for the local connector (#1245) as part of this PR, or leave it for a separate one?

We can do it here in a separate commit, or in another PR. Whatever works best for you, thanks!

@antross antross referenced this pull request Aug 24, 2018

Closed

New: Add extension for VSCode #1254

14 of 15 tasks complete
@antross

This comment has been minimized.

Member

antross commented Aug 24, 2018

Ok, I'll leave the local connector documentation for another PR (likely once we've fixed it to parse HTML).

@molant

molant approved these changes Aug 28, 2018

LGTM, @sarvaje can you take a look?

@@ -97,7 +98,7 @@ export default class LocalConnector implements IConnector {
*/
const uri: url.URL = getAsUri(target);
const filePath: string = asPathString(uri);
const content: NetworkData = await this.fetchContent(filePath);
const content: NetworkData = await this.fetchContent(filePath, options);

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

await this.fetchContent(filePath, null, options);

otherwise, when you check in line # 253 options.content is going to be always null.

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

Good catch, thanks!

// Ignore options.content when matching multiple files
if (options && options.content) {
delete options.content;

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

why don't set just to null?

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

No reason other than preference. I'll change this to set content to null instead.

Chore: Fix issues raised in PR feedback
Also fixed the new test case which was incorrect too.
@@ -874,3 +874,46 @@ test.serial('executeOn should return all messages', async (t) => {
t.is(result.length, 2);
});
test.serial('executeOn should forward content if provided', async (t) => {

This comment has been minimized.

@alrra

alrra Aug 28, 2018

Member

We started using ' around coding related things such as 'executeOn', so let's use that in the future.

I'll make this change before merging.

@alrra alrra closed this in 20016e5 Aug 28, 2018

alrra added a commit that referenced this pull request Aug 28, 2018

Fix: Allow to pass content directly to things such as local connector
This change is done in order to allow things such as extensions
(e.g. VSCode extension).

Close #1268

@antross antross deleted the antross:pass-content branch Oct 14, 2018

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