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

add blank rules file to build-pkg, use old rules file as example #196

Merged
merged 4 commits into from
Nov 19, 2020

Conversation

asdvalenzuela
Copy link
Contributor

@asdvalenzuela asdvalenzuela commented Nov 18, 2020

In adding more detail to the getting started docs, we noticed that the rules file had been left out of the build package. Since the existing rules file was more of an example file, and so as not to pollute the config with those examples, we renamed rules.toml to rules_complete.toml. Since defaults for the rules configuration are set in file_config.go, rules.toml is now blank until folks start adding their own rules.

Drive-by cleanup of a few things in file_config.go and regarding our config tests.

@asdvalenzuela asdvalenzuela requested a review from a team November 18, 2020 23:32
@asdvalenzuela asdvalenzuela changed the title add rules file to build-pkg [WIP] add rules file to build-pkg Nov 18, 2020
@asdvalenzuela asdvalenzuela added the wip Work In Progress label Nov 18, 2020
@asdvalenzuela asdvalenzuela changed the title [WIP] add rules file to build-pkg add blank rules file to build-pkg, use old rules file as example Nov 19, 2020
@asdvalenzuela asdvalenzuela removed the wip Work In Progress label Nov 19, 2020
@@ -146,6 +143,8 @@ func NewConfig(config, rules string, errorCallback func(error)) (Config, error)

r.SetDefault("Sampler", "DeterministicSampler")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does these need to happen after the config has been loaded? Could they all be done beforehand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't need to happen after. I think this bit is just meant to continue the distinction between the general config file and the rules file, since these values are all set in the rules file. @martin308 Is that correct?

Copy link
Member

Choose a reason for hiding this comment

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

we need to do them beforehand AFAIK, which is the case here as we don't load the config until line 150

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, just seen the SetDefaults are on the r not the c object so can't happen any sooner 👍

@@ -146,6 +143,8 @@ func NewConfig(config, rules string, errorCallback func(error)) (Config, error)

r.SetDefault("Sampler", "DeterministicSampler")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, just seen the SetDefaults are on the r not the c object so can't happen any sooner 👍

Copy link
Member

@martin308 martin308 left a comment

Choose a reason for hiding this comment

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

@asdvalenzuela asdvalenzuela merged commit dfcec54 into main Nov 19, 2020
@asdvalenzuela asdvalenzuela deleted the alaina.add-rules-file-to-build-pkg branch November 19, 2020 22: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.

3 participants