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 support for environments #244

Merged
merged 2 commits into from
Apr 7, 2022
Merged

Add support for environments #244

merged 2 commits into from
Apr 7, 2022

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Apr 4, 2022

Which problem is this PR solving?

  • Adds support for environments.

Closes #238

Short description of the changes

  • Add isClassic function
  • Test in validateEvent (called for every event)
  • Cleans up some error messages
  • Adds some tests
  • Does nothing new on construction of a builder

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good so far - added some small suggestions to remove the . prefix from error messages.

src/libhoney.js Outdated
if (typeof dataset !== "string" || dataset === "") {
console.error(".dataset must be a non-empty string");
if (typeof dataset !== "string") {
console.error(".dataset must be a string");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.error(".dataset must be a string");
console.error("dataset must be a string");

src/libhoney.js Outdated
return null;
}

if (dataset === "") {
if (this.isClassic(writeKey)) {
console.error(".dataset must be a non-empty string");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.error(".dataset must be a non-empty string");
console.error("dataset must be a non-empty string");


expect(transmission.events).toHaveLength(0);
expect(console.error.mock.calls[0][0]).toBe(
".dataset must be a non-empty string"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
".dataset must be a non-empty string"
"dataset must be a non-empty string"

@kentquirk kentquirk marked this pull request as ready for review April 7, 2022 14:44
@kentquirk kentquirk requested a review from a team April 7, 2022 14:44
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good - thanks @kentquirk

@kentquirk kentquirk changed the title first pass at supporting environments Add support for environments Apr 7, 2022
@kentquirk kentquirk merged commit 56e8e5a into main Apr 7, 2022
@kentquirk kentquirk deleted the kent.env_support branch April 7, 2022 14:51
@kentquirk kentquirk mentioned this pull request Apr 7, 2022
kentquirk added a commit that referenced this pull request Apr 7, 2022
Release PR for 3.1.0

### Enhancements

-   Add support for environments (#244) | [@kentquirk](https://github.com/kentquirk)
-   ci: add node 17 to test matrix (#195) | [@vreynolds](https://github.com/vreynolds)
-   empower apply-labels action to apply labels (#205) | [@robbkidd](https://github.com/robbkidd)

### Maintenance

-   gh: add re-triage workflow (#215) | [@vreynolds](https://github.com/vreynolds)
-   Update dependabot.yml (#212) | [@vreynolds](https://github.com/vreynolds)
-   Remove husky and lint-staged
-   Bump @babel/core to 7.16.12 (#230)
-   Bump @babel/eslint-parser to 7.16.5 (#223)
-   Bump @babel/preset-env to 7.16.8 (#224)
-   Bump @rollup/plugin-commonjs to 21.0.1 (#197)
-   Bump @rollup/plugin-node-resolve to 13.1.3 (#225)
-   Bump @rollup/plugin-replace to 3.0.1 (#216)
-   Bump eslint to 8.6.0 (#227)
-   Bump minimist to 2.5.1 (#220)
-   Bump superagent from 6.1.0 to 7.0.2 (#226)
-   Bump vm2 to 3.9.7 (#234)
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.

Update for E&S
2 participants