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

Update Precendence order documentation according implementation #314

Merged
merged 1 commit into from Mar 6, 2020

Conversation

zaverden
Copy link
Contributor

@zaverden zaverden commented Aug 19, 2019

Fixes #211
Fixes #208

@coveralls
Copy link

coveralls commented Aug 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 18b9c89 on zaverden:patch-1 into 9f2b2d3 on mozilla:master.

@zaverden zaverden mentioned this pull request Aug 19, 2019
@zaverden zaverden changed the title Update Precendence order according implementation Update Precendence order documentation according implementation Sep 9, 2019
@zaverden
Copy link
Contributor Author

zaverden commented Sep 9, 2019

@A-312 could you check this out please? this is a documentation update according our discussion in #211. Does it look OK, or I should change it to be more clear?

@A-312
Copy link
Contributor

A-312 commented Sep 9, 2019

I don't know, after looking this part, I don't like the current integration. We should have something like that : config.loadExternal(['env', 'arg']). Or rewrite all the content of this paragraph.

After a long reflection, I think this can be better :

When merging configuration values from different sources, Convict follows precedence rules. The order, from lowest to highest, for config.loadFile(file) and config.load(json) is :

  1. Default value ;
  2. File or json set in function argument ;
  3. Environment variables (only used when env property is set in schema) ;
  4. Command line arguments (only used when arg property is set in schema).

Environment variables and Command line arguments can be overridden using the env or args option of the convict function convict(schema, {env: {}, arg: {}}) ???? <--- check this part

I don't understand : can be overridden using the env|args option of the convict function -> I think this part is not in the doc. If I understand we can do convict(schema, opt), is not it ?

We speak only about loadFile and load because set have priority only if is call after load because :

const config = convict({ port: { default: 3000 } }
config.load({ port: 8080 });
console.log(config.get('port')); // 8080
config.set('port', 433);
console.log(config.get('port')); // 443
config.load({ port: 9000 });
console.log(config.get('port')); // 9000

@zaverden
Copy link
Contributor Author

@A-312 Thank you for a review. I did not think about complete rewrite of this block, but it make sense in order to make it clear.

I'll update the PR later

@A-312
Copy link
Contributor

A-312 commented Sep 28, 2019

ping @zaverden

@zaverden
Copy link
Contributor Author

zaverden commented Oct 8, 2019

@A-312 sorry for delay.

I have updated description and moved can be overridden using the env|args option to a separate section

@A-312
Copy link
Contributor

A-312 commented Dec 21, 2019

This PR is ready

@A-312
Copy link
Contributor

A-312 commented Dec 28, 2019

@madarche This one is ready

@A-312 A-312 mentioned this pull request Dec 28, 2019
@A-312
Copy link
Contributor

A-312 commented Jan 1, 2020

@zaverden Can you rebase your PR in mozilla:feat-multi-packages-split ? Is the branch for the next update (6.0.0).

@zaverden zaverden changed the base branch from master to feat-multi-packages-split January 3, 2020 17:48
@zaverden
Copy link
Contributor Author

zaverden commented Jan 3, 2020

@A-312 done

@A-312 A-312 mentioned this pull request Jan 5, 2020
14 tasks
@madarche madarche merged commit 946ead8 into mozilla:feat-multi-packages-split Mar 6, 2020
madarche pushed a commit that referenced this pull request Mar 6, 2020
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.

None yet

4 participants