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 clean-up: class-methods-use-this #1312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lindapaiste
Copy link
Contributor

Fixes some violations of eslint rule class-methods-use-this which were suppressed.

  • Use the isAbsoluteUrl and getModelPath utilities from modelLoader instead of duplicating them in multiple classes.
  • Removed one eslint-disable which was not actually needed.
  • Use boolean || instead of class method checkUndefined to set defaults in FaceApi. I wanted to use the nullish coalescing ?? but the eslint version is too outdated to support it!

The FaceApi code could be cleaned up further with object spreading, but the behavior would differ slightly if the user explicitly provides a falsey value. It could be reduced to:

this.config = {
  ...DEFAULTS,
  ...options,
  MODEL_URLS: {
    ...DEFAULTS.MODEL_URLS,
    ...options.MODEL_URLS
  }
}

…ere suppressed.

* Use the `isAbsoluteUrl` and `getModelPath` utilities from `modelLoader` instead of duplicating them in multiple classes.
* Removed one `eslint-disable` which was not actually needed.
* Use boolean `||` instead of `checkUndefined` in `FaceApi`. (This could be cleaned up further with object spreading, but the behavior would differ slightly if the user explicitly provides a falsey value.)
@joeyklee joeyklee added the SEMVER/patch a version tag for a patch release change label Mar 4, 2022
Copy link
Contributor

@joeyklee joeyklee left a comment

Choose a reason for hiding this comment

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

LGTM! 🏅 Thanks for digging in here.

@lindapaiste
Copy link
Contributor Author

lindapaiste commented May 4, 2022

Let's hold off on merging this one because I really don't love the || in FaceApi. ?? would have been better, but I want to create a general utility for merging options with defaults.

As for the isAbsoluteUrl method in DCGAN, that is addressed in #1376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SEMVER/patch a version tag for a patch release change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants