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

feat: update default bots folder #1548

Merged
merged 13 commits into from
Nov 19, 2019
Merged

Conversation

liweitian
Copy link
Contributor

@liweitian liweitian commented Nov 11, 2019

Description

  1. The default folder for creating a new bot is path.join(os.homedir(), 'Documents', 'Composer') now.
  2. If the last accessed path is invalid, load default folder.

Task Item

Closes #1506
Closes #1363

Type of change

Bug fix (non-breaking change which fixes an issue)

@liweitian liweitian force-pushed the liweitian/updateDefaultFolder branch from eee3744 to 620d3fb Compare November 11, 2019 09:12
@a-b-r-o-w-n a-b-r-o-w-n changed the title Liweitian/update default folder feat: update default bots folder Nov 12, 2019
@@ -18,6 +18,9 @@ class StorageService {

constructor() {
this.storageConnections = Store.get(this.STORE_KEY);
this.storageConnections.forEach(s => {
this.createFolderRecurively(s.defaultPath);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the user has changed their default folder and then deleted the one we create? Won't this create /Documents/Composer every time?

Copy link
Contributor Author

@liweitian liweitian Nov 14, 2019

Choose a reason for hiding this comment

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

the default path is set in file data.template.json. It does not allow users to change it. These lines of code is to make sure the default path is always valid.

Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

I have some questions about edge cases.

@liweitian liweitian force-pushed the liweitian/updateDefaultFolder branch from 8044e2b to d0eb0b2 Compare November 14, 2019 08:21
if (fs.existsSync(s.path)) {
temp.path = Path.resolve(s.path); // resolve path if path is relative, and change it to unix pattern
} else {
this.createDefaultBotFolders();
Copy link
Contributor

Choose a reason for hiding this comment

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

you are creating all folders inside a for loop. It's a O(n2) operation.

my suggestion is call this method "ensureFolderExisit", and put this after this map.

@cwhitten
Copy link
Member

@liweitian there is a lint failure

Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

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

needs to address lint issue

@liweitian liweitian force-pushed the liweitian/updateDefaultFolder branch from c6468d5 to eb0cde2 Compare November 19, 2019 06:39
@cwhitten cwhitten merged commit 8f2bc2e into master Nov 19, 2019
@cwhitten cwhitten deleted the liweitian/updateDefaultFolder branch November 19, 2019 18:07
a-b-r-o-w-n pushed a commit to a-b-r-o-w-n/BotFramework-Composer that referenced this pull request Nov 20, 2019
a-b-r-o-w-n pushed a commit that referenced this pull request Nov 20, 2019
@liweitian liweitian restored the liweitian/updateDefaultFolder branch November 21, 2019 08:51
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.

4 participants