Skip to content
This repository was archived by the owner on Feb 19, 2025. It is now read-only.

Conversation

@r2o3k
Copy link

@r2o3k r2o3k commented Feb 12, 2024

No description provided.

Copy link
Contributor

@EandrewJones EandrewJones left a 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)

@Jyyjy
Copy link
Contributor

Jyyjy commented Feb 12, 2024

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

@EandrewJones
Copy link
Contributor

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.

settings.logCountThreshold = +get('data-threshold') || 5;
settings.userId = get('data-user') || null;
settings.version = get('data-version') || null;
settings.toolVersion = get('data-version') || null;
Copy link
Contributor

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').

@EandrewJones EandrewJones mentioned this pull request Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants