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

A bit of cleanup #2

Merged
merged 21 commits into from Jan 10, 2017
Merged

A bit of cleanup #2

merged 21 commits into from Jan 10, 2017

Conversation

dylanegan
Copy link
Contributor

This is just to clean things up a bit as there were remnants from the Zendesk code that wasn't necessary for this library.

  • Remove Zendesk referenced configuration
  • Add Standard JS ESLint support
  • Adhere to more npm standard packaging and provide distributions for various things

Also the specifications weren't running properly, so I've fixed those to run properly now.

@dylanegan
Copy link
Contributor Author

Ignore the large diff count, that is just yarn.lock for the most part.

@dylanegan
Copy link
Contributor Author

I've also added more CRUD support here.

@dylanegan
Copy link
Contributor Author

@aossowski shall we merge this to master and maybe publish to NPM?

@aossowski
Copy link
Contributor

@dylanegan we can merge to master but I think too many resources are still missing there to publish it

@dylanegan
Copy link
Contributor Author

Is that an issue? If it's pre-1.0.0 I wouldn't expect feature parity.

@@ -27,6 +27,7 @@
"test": "./node_modules/karma/bin/karma start --single-run"
},
"devDependencies": {
"babel-cli": "^6.18.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the requirement here?

Copy link
Contributor

Choose a reason for hiding this comment

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

npm run build didn't work without it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the output without it?

Copy link
Contributor

Choose a reason for hiding this comment

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

babel: command not found ;-)

Copy link
Contributor Author

@dylanegan dylanegan Jan 9, 2017

Choose a reason for hiding this comment

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

@aossowski no problem, was able to replicate. I must've done nom install -g babel-cli as some point on my old machine as my new machine failed immediately. Thanks!

}

get () {
return this.__client.request(this.__urlFor(''))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be able to call it without the argument here.

@dylanegan
Copy link
Contributor Author

@aossowski do you foresee any additional endpoints we'll need for Reporting or would this be safe to merge in and move things to using it?

@aossowski
Copy link
Contributor

@dylanegan we will need more endpoints to save/retrieve reports and export data but we can add it later as separate PRs

@dylanegan dylanegan merged commit 5cc31ed into master Jan 10, 2017
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.

None yet

2 participants