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

Code changes for toolName and toolVersion constraints #411

Open
wants to merge 4 commits into
base: test
Choose a base branch
from

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)

src/UserALEWebExtension/background.js Outdated Show resolved Hide resolved
src/getInitialSettings.js Outdated Show resolved Hide resolved
@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.

@@ -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;
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 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