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.

Sign up for GitHub

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

Jump to bottom

Get default config options when not specified in user config #2126

Merged
merged 11 commits into from
Sep 6, 2017
Merged

Get default config options when not specified in user config #2126

merged 11 commits into from
Sep 6, 2017

Conversation

okonek
Copy link
Contributor

@okonek okonek commented Aug 30, 2017

App uses options from default config when not specified in user config. That's issue #1588. Now if you won't specify an option in your config, it will be automatically added from default config.

@okonek okonek mentioned this pull request Aug 30, 2017
Closed
@okonek
Copy link
Contributor Author

okonek commented Aug 30, 2017

Anyone?

albinekb
albinekb approved these changes Sep 1, 2017
View reviewed changes
Copy link
Contributor

@albinekb albinekb left a comment

Choose a reason for hiding this comment

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

Looks 👌 to me, @ppot?

@ppot
Copy link
Contributor

ppot commented Sep 1, 2017

@albinekb I don't know if it's a good Idea to use deepmerge. If we go in the way to allow only custom flag. Since this is what it is. Then in this way we need to remove the whole install process of .hyper.js file. And only read and assign base on available flags. For this. I would say no on this part.

chabou
chabou suggested changes Sep 1, 2017
View reviewed changes
Copy link
Collaborator

@chabou chabou left a comment

Choose a reason for hiding this comment

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

'lodash/merge' is better.
Do not forget to commit your yarn.lock file too ;)

app/config/init.js Outdated
@@ -1,4 +1,5 @@
const vm = require('vm');
const deepmerge = require('deepmerge');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to use 'lodash/merge' here. More widely used.

@okonek
Copy link
Contributor Author

okonek commented Sep 2, 2017

I'll change tomorrow

chabou
chabou suggested changes Sep 3, 2017
View reviewed changes
Copy link
Collaborator

@chabou chabou left a comment

Choose a reason for hiding this comment

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

merge is not lodash/merge :)

@chabou
Copy link
Collaborator

chabou commented Sep 3, 2017

Oh I think I understood.
When I used lodash/merge notation, it didn't mean lodash or merge.
Add lodash dependency and cherry-pick merge method when requiring:
const merge = require('lodash/merge');

@chabou
Copy link
Collaborator

chabou commented Sep 3, 2017

And please use yarn. npm@5 is ok but our project use yarn.

@okonek
Copy link
Contributor Author

okonek commented Sep 3, 2017

Ok. :) I understand like lodash or merge

@chabou
Copy link
Collaborator

chabou commented Sep 3, 2017

Can you remove package-lock.json files and resolve conflicts?

chabou
chabou reviewed Sep 3, 2017
View reviewed changes
app/package.json Outdated
@@ -19,6 +19,8 @@
"electron-config": "1.0.0",
"electron-is-dev": "0.3.0",
"electron-squirrel-startup": "1.0.0",

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this extra blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok :)

@okonek
Update package.json
a468306 
Removed blank line
@okonek
Copy link
Contributor Author

okonek commented Sep 5, 2017

Anything else?

@chabou
Copy link
Collaborator

chabou commented Sep 5, 2017

Can you remove your package-lock.json files please?
(Sorry for the delay)

@okonek
Copy link
Contributor Author

okonek commented Sep 6, 2017

Done

@chabou chabou merged commit be5786e into vercel:master Sep 6, 2017
@chabou
Copy link
Collaborator

chabou commented Sep 6, 2017

Thank you so much for your help 🙏

@chabou chabou changed the title Using options from default config when not specified in user config using DeepMerge Get default config options when not specified in user config Sep 6, 2017
@chabou chabou mentioned this pull request Sep 6, 2017
Closed
@okonek
Copy link
Contributor Author

okonek commented Sep 8, 2017

Thank You too. I learned how is it to contribute to open-source project.

chabou added a commit that referenced this pull request Sep 10, 2017
@chabou
Merge branch 'master' into xterm
0fbf7cf 
* master:
  Add default keymaps reference to documentation (#2185)
  Add category option under linux key in root package.json (#2184)
  Add vscode debug config, update readme (#2181)
  Get default config options when not specified in user config (#2126)
  Fix session cleaning (#2176)
  Delete .DS_Store file (#2171)
@chabou chabou mentioned this pull request Sep 13, 2017
Closed
chabou added a commit to chabou/hyper that referenced this pull request Sep 13, 2017
@chabou
Revert vercel#2126
de76d56
@zimme zimme mentioned this pull request Sep 14, 2017
Merged
timothyis pushed a commit that referenced this pull request Sep 15, 2017
@chabou @codetheory
Revert #2126 (#2202)
97d1ae3
chabou added a commit that referenced this pull request Sep 16, 2017
@chabou
Merge branch 'master' into v2
507fc28 
* master:
  Revert #2126 (#2202)
  Use child_process.execFile to prevent unescaped stuff (#2206)
  1.4.4
spncrgr pushed a commit to spncrgr/hyper that referenced this pull request Sep 23, 2017
@okonek @spncrgr
Get default config options when not specified in user config (vercel#…
5ea9b46 
…2126)
spncrgr pushed a commit to spncrgr/hyper that referenced this pull request Sep 23, 2017
@chabou @spncrgr
Revert vercel#2126 (vercel#2202)
1f9ac91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chabou chabou chabou approved these changes

@albinekb albinekb albinekb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@okonek @ppot @chabou @albinekb