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

WIP : #258 #666

Merged
merged 7 commits into from
Oct 9, 2018
Merged

WIP : #258 #666

merged 7 commits into from
Oct 9, 2018

Conversation

tkrtmy
Copy link

@tkrtmy tkrtmy commented Oct 4, 2018

This is proposal.

Customized sync problem is each machine's path difference.
So be able to set up download path at each machine.

and resolve ↓ issue.
#258

Scenario

1. Add additionaly sync files.

2018-10-04 13 15 19

2018-10-04 13 17 31

syncLocalSettings.json

{
 ...
  "customFiles": {
    ".eslint": "/Users/tkrtmy/.eslint" // download  path. if is not set it, is not downloaded it.
  },
 ...
}

append |customized_sync| prefix to file's name at gist for identifiyng
ex. .eslint => |customized_sync|.eslint

2. upload or download them.

@@ -193,4 +195,43 @@ export class FileService {
throw err;
}
}

Copy link
Author

Choose a reason for hiding this comment

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

add some util methods

@@ -188,6 +188,18 @@ export class Sync {
return matchedFolders.length > 0;
});
}
const customFileKeys: string[] = Object.keys(
Copy link
Author

Choose a reason for hiding this comment

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

upload additional files

@@ -437,7 +449,21 @@ export class Sync {
keys.forEach(gistName => {
if (res.data.files[gistName]) {
if (res.data.files[gistName].content) {
if (gistName.indexOf(".") > -1) {
const prefix = FileService.CUSTOMIZED_SYNC_PREFIX;
Copy link
Author

Choose a reason for hiding this comment

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

download additional files

const fileName = gistName.split(prefix).join(""); // |customized_sync|.htmlhintrc => .htmlhintrc
if (!(fileName in customSettings.customFiles)) {
// syncLocalSettings.json > customFiles doesn't have key
return;
Copy link
Author

Choose a reason for hiding this comment

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

skip download if does not set download path.

@@ -699,6 +730,7 @@ export class Sync {
"cmd.otherOptions.toggleAutoDownload",
"cmd.otherOptions.toggleSummaryPage",
"cmd.otherOptions.preserve",
"cmd.otherOptions.customizedSync",
Copy link
Author

Choose a reason for hiding this comment

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

create new advanced option

@shanalikhan
Copy link
Owner

There should be two options

  1. Sync : Upload Custom File
  2. Sync : Download Custom File

Uploading the custom file is the same you have done so far.

Download custom file when selected The menu should be opened to select the custom files available in the GIST.
Upon selecting the file name it should download the file into root directory of the opened workspace/project.

In this way usr can have multiple files to upload from multiple computers in single GIST
But only download or import the selected file into the current workspace

@shanalikhan shanalikhan changed the title Customized Sync WIP : #258 Oct 6, 2018
@tkrtmy
Copy link
Author

tkrtmy commented Oct 7, 2018

@shanalikhan Thank you for your review.
I understood that Download custom file option is required.
So I'll add Sync: Import Custom File to workspace (download in root dir of the workspace) in Advanced Options.
Did I get the right?

@tkrtmy
Copy link
Author

tkrtmy commented Oct 7, 2018

Added download custom file option.
2018-10-07 22 41 12

select customized file to download from gist.
2018-10-07 22 41 25

}
}
},
9: async () => {
Copy link
Author

Choose a reason for hiding this comment

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

download customized file from gist option

@tkrtmy
Copy link
Author

tkrtmy commented Oct 7, 2018

screenshot of final draft.
2018-10-07 23 27 00

@shanalikhan
Copy link
Owner

Thanks for your work.
Meanwhile, i will be doing a code review, can you write the test using mocha also for this feature.

@protiumx
Copy link
Contributor

protiumx commented Oct 8, 2018

@tkrtmy I added tests in the PR #667. I used mocha & chai. To avoid conflicts, @shanalikhan could you define a folder structure for tests? As reference test folder in PR

@shanalikhan
Copy link
Owner

I have defined the test folder in this branch with index.ts default file.
You can write tests over there.

@tkrtmy
Copy link
Author

tkrtmy commented Oct 8, 2018

@ioprotium I think that I should use this.
vscode/lib/testrunner use mocha.
How about it?

for example directory

test
├── index.ts
├── extension.test.ts
└── service
    └── fileService.test.ts

index.ts like this.

import * as testRunner from "vscode/lib/testrunner";

testRunner.configure({
  ui: "bdd",
  useColors: true,
  timeout: 5000
});

module.exports = testRunner;

@shanalikhan Sorry, In my PR. New feature is monolithically built in sync.ts, not as module. so I can add small test.
I can't write sync.ts's test because of time constrains. 😭

@shanalikhan
Copy link
Owner

shanalikhan commented Oct 8, 2018

so I can add small test.

No problem, you can write whatever test you like regarding above feature 😄

Public Wiki edit is enabled. Feel free to create new page with your work explained.

@shanalikhan shanalikhan added this to the v3.2 milestone Oct 9, 2018
@shanalikhan shanalikhan merged commit 9e287f8 into shanalikhan:v3.2 Oct 9, 2018
@shanalikhan
Copy link
Owner

@tkrtmy
I have merged this Pull Request if i will highly appreciate if you can write the Test Cases for this feature.

I will publish new version in a week or two.

@tkrtmy tkrtmy mentioned this pull request Oct 11, 2018
@tkrtmy
Copy link
Author

tkrtmy commented Oct 11, 2018

@shanalikhan
I edit this wiki.

And wrote small Test Cases regarding this feature.
#671

@shanalikhan
Copy link
Owner

shanalikhan commented Oct 11, 2018

Thanks, Merged!
Will, you be able to solve the issues for this feature, if we get any after publishing. ?

@tkrtmy
Copy link
Author

tkrtmy commented Oct 15, 2018

You're welcome.
Please any issue related with this feature will be assign to me.

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

Successfully merging this pull request may close these issues.

3 participants