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

refactor: Remove util.inherits #70 #109

Merged
merged 23 commits into from
Jun 8, 2023

Conversation

dsschiramm
Copy link

@dsschiramm dsschiramm commented Dec 18, 2021

Summary

Remove util.inherits
Use ES6 Class syntax with extends instead of utils.inherits

Linked issue(s)

#70

Involved parts of the project

Added tests?

Yes, added test to validate OAuthError constructor and properties in test/unit/errors/oauth-error_test.js

OAuth2 standard

Reproduction

Checkout this branch and run the tests.

@dsschiramm dsschiramm changed the base branch from master to development December 18, 2021 22:19
Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

If you follow my comments below the tests will all pass

lib/errors/access-denied-error.js Outdated Show resolved Hide resolved
lib/errors/oauth-error.js Outdated Show resolved Hide resolved
lib/errors/oauth-error.js Outdated Show resolved Hide resolved
lib/errors/oauth-error.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 18, 2021

Also please run one time "npm run lint:fix" so that all linting errors go away.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 18, 2021

@dsschiramm
Why did you close this?

@dsschiramm
Copy link
Author

@dsschiramm Why did you close this?

I made a mistake, I'm fixing it, I'll resubmit the pull request later.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 18, 2021

You could have kept it open or made it a draft. Then everytime you would push into your repo/branch it would automatically show up here.

@dsschiramm
Copy link
Author

You could have kept it open or made it a draft. Then everytime you would push into your repo/branch it would automatically show up here.

Got it, thanks I didn't know, I'll open it and make the corrections.

@dsschiramm dsschiramm reopened this Dec 18, 2021
@dsschiramm
Copy link
Author

@Uzlopak could you review one more time ?

@jankapunkt
Copy link
Member

Hey @dsschiramm thanks a lot for your contribution. Can you please edit your PR and fill out the template sections (summary, linked issues etc.) so we as reviewers can easily step in see what's going on.

@jankapunkt jankapunkt linked an issue Dec 19, 2021 that may be closed by this pull request
@jankapunkt jankapunkt added the code quality 🧽 Relating to code quality label Dec 19, 2021
lib/errors/oauth-error.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jankapunkt
Copy link
Member

@dsschiramm we are back in the new year, your PR is not forgotten ;-)

@jankapunkt
Copy link
Member

I will first create the 4.2.0 release, because it contains important fixes. After that we can check on code-improvements as this one.

@jankapunkt jankapunkt added this to the v5 milestone Mar 29, 2022
@HappyZombies HappyZombies added the on hold 🛑 We will look into it at a later time label Jun 4, 2022
daniel.santos and others added 2 commits July 22, 2022 02:12
# Conflicts:
#	lib/grant-types/refresh-token-grant-type.js
#	package-lock.json
jankapunkt
jankapunkt previously approved these changes Mar 21, 2023
@jankapunkt
Copy link
Member

@Uzlopak as far as I can see all changes were addressed and this can be merged, once you approve

@jankapunkt jankapunkt added dependencies 🔌 Pull requests that update a dependency file completed 😀 Work that has been completed and removed on hold 🛑 We will look into it at a later time labels Mar 21, 2023
@jankapunkt jankapunkt modified the milestones: v5, v4.4 Mar 21, 2023
@jankapunkt
Copy link
Member

@Uzlopak friendly ping

Uzlopak
Uzlopak previously approved these changes May 26, 2023
Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

pong ;)

LGTM

jankapunkt and others added 9 commits May 30, 2023 10:38
Bumps [sinon](https://github.com/sinonjs/sinon) from 13.0.1 to 15.1.0.
- [Release notes](https://github.com/sinonjs/sinon/releases)
- [Changelog](https://github.com/sinonjs/sinon/blob/main/docs/changelog.md)
- [Commits](sinonjs/sinon@v13.0.1...v15.1.0)

---
updated-dependencies:
- dependency-name: sinon
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [chai](https://github.com/chaijs/chai) from 4.3.4 to 4.3.7.
- [Release notes](https://github.com/chaijs/chai/releases)
- [Changelog](https://github.com/chaijs/chai/blob/4.x.x/History.md)
- [Commits](chaijs/chai@v4.3.4...v4.3.7)

---
updated-dependencies:
- dependency-name: chai
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/sinon-15.1.0

build(deps-dev): bump sinon from 13.0.1 to 15.1.0
Bumps [mocha](https://github.com/mochajs/mocha) from 9.2.2 to 10.2.0.
- [Release notes](https://github.com/mochajs/mocha/releases)
- [Changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
- [Commits](mochajs/mocha@v9.2.2...v10.2.0)

---
updated-dependencies:
- dependency-name: mocha
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/mocha-10.2.0

build(deps-dev): bump mocha from 9.2.2 to 10.2.0
…yarn/chai-4.3.7

build(deps-dev): bump chai from 4.3.4 to 4.3.7
Bumps [eslint](https://github.com/eslint/eslint) from 8.4.1 to 8.41.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.4.1...v8.41.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/eslint-8.41.0

build(deps-dev): bump eslint from 8.4.1 to 8.41.0
@jankapunkt jankapunkt changed the base branch from development to master June 5, 2023 08:15
@jankapunkt jankapunkt dismissed stale reviews from Uzlopak and themself June 5, 2023 08:15

The base branch was changed.

dependabot bot and others added 3 commits June 5, 2023 18:59
Bumps [eslint](https://github.com/eslint/eslint) from 8.41.0 to 8.42.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.41.0...v8.42.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/eslint-8.42.0

build(deps-dev): bump eslint from 8.41.0 to 8.42.0
@jankapunkt jankapunkt changed the base branch from master to development June 6, 2023 09:28
@jankapunkt jankapunkt merged commit 9fd04f6 into node-oauth:development Jun 8, 2023
@jankapunkt
Copy link
Member

@dsschiramm it's finally merged 🎉 thanks a lot for your contribution and patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality 🧽 Relating to code quality completed 😀 Work that has been completed dependencies 🔌 Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove util.inherits
4 participants