-
Notifications
You must be signed in to change notification settings - Fork 26
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
Code changes for toolName and toolVersion constraints #411
base: test
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these minor changes and then we need to have tests pass.
Beleive @Jyyjy has to trigger tests. (note to self we really need to get automated checks in place)
Any committer can trigger tests, and they will automatically trigger if a committer makes a PR. Of course you also have to link your Apache id to your GitHub account |
I'm not yet a committer. I submitted my paperwork but still awaiting things like apache id. |
@@ -42,7 +42,7 @@ export function getInitialSettings() { | |||
settings.transmitInterval = +get('data-interval') || 5000; | |||
settings.logCountThreshold = +get('data-threshold') || 5; | |||
settings.userId = get('data-user') || null; | |||
settings.version = get('data-version') || null; | |||
settings.toolVersion = get('data-version') || null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where I was referencing changing the script tag to match the setting name.
so it'd be:
get('data-tool-version')
This is a breaking change, because it changes the script-tag api. And since toolVersion is required, the script would throw if users did not set this field.
We should add a default besides null
to keep it from breaking (e.g. '1.0').
No description provided.