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

Settings schema #48

Merged
merged 11 commits into from
Feb 1, 2018
Merged

Settings schema #48

merged 11 commits into from
Feb 1, 2018

Conversation

samussiah
Copy link
Contributor

@samussiah samussiah commented Jan 18, 2018

Issues

Closes #45

@samussiah samussiah requested a review from jwildfire January 18, 2018 21:27
Copy link
Contributor

@jwildfire jwildfire left a comment

Choose a reason for hiding this comment

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

Minor tweaks likely needed after changes to #47.

@@ -20,10 +20,11 @@
],
"dependencies": {
"d3": "^3",
"webcharts": "^1.9"
"webcharts": "^1"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good catch

controls initial display of unscheduled visits

**default:** none

Copy link
Contributor

Choose a reason for hiding this comment

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

default = false, no?


# settings.unscheduled_visit_pattern
`string`

Copy link
Contributor

Choose a reason for hiding this comment

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

is this a string? or a regular expression? (maybe json schema only supports strings?)

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind - i see the logic below dealing with this.


**default:** `"/unscheduled|early termination/i"`


Copy link
Contributor

Choose a reason for hiding this comment

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

update based on comments from #47

);
settings.unscheduled_visit_regex = new RegExp(pattern, flags);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good - should also make it easier to support config.unscheduled_visit_list as suggested in #47 code review

@samussiah
Copy link
Contributor Author

Ready for you @jwildfire

const defaultSettings = {
//Custom settings for this template
import merge from './util/merge';

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I totally see the point of splitting the settings here, but I guess it doesn't hurt anything ... so 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to pull in the webcharts settings for the configuration wiki and this was the hacky route I chose.

//Warn user of non-numeric endpoints.
if (catMeasures.length)
//Warn user of removed records.
if (nRemoved > 0)
console.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned elsewhere, we should move this to a footnote. Created #50

@samussiah samussiah requested a review from dschwentker January 29, 2018 15:20
@samussiah samussiah self-assigned this Jan 29, 2018
@samussiah samussiah added this to the v2.2.0 milestone Jan 29, 2018
@jwildfire jwildfire requested review from brittsikora and removed request for dschwentker January 30, 2018 17:33
Copy link

@brittsikora brittsikora left a comment

Choose a reason for hiding this comment

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

Feature testing passes

@samussiah samussiah merged commit 8aadcff into v2.2.0-dev Feb 1, 2018
@samussiah samussiah deleted the settings-schema branch February 1, 2018 22:35
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