refactored logout-service #319
Conversation
kevinlaiGH
commented
Nov 7, 2023
kevinlaiGH
commented
- Switch to private properties.
- Use async/await consistently instead of mixing with .catch.
- Use a constant for the maximum check count instead of a magic number.
|
Thanks for this @kevinlaiGH. Appreciate the contribution! That said, can you please remove all the semicolons. This project somewhat follows 'standardjs' styling (though I need to get it added as part of the build/linting steps). Also, can you remove the private functions. I'm not a fan of them in typescript/js code since JavaScript doesn't have a true concept of private functions. Also this is an open source library so I err on keeping functions public. (I personally hate when I need to do a simple monkey patch or method overload and can't because the library writer protected functions) Otherwise, much better removing the |
|
Also, if you are interested in cleaning up style/linting, we really need a contribution to add standardjs, but for typescript. Would love that if you are interested in contributing more! https://standardjs.com/ |
|
Thanks for the suggestions @motdotla. I have changed it accordingly. |
src/services/logout-service.ts
Outdated
| if (!this.yes) { | ||
| this.log.local(`Logout URL: ${this.logoutUrl}`) | ||
| const answer = await CliUx.ux.prompt(`${chalk.dim(this.log.pretextLocal)}Press ${chalk.green('y')} (or any key) to logout and revoke credential (.env.me) or ${chalk.yellow('q')} to exit`) | ||
| if (answer === 'q' || answer === 'Q') { | ||
| if (answer.toLowerCase() === 'q') { |
There was a problem hiding this comment.
can we have this also strip any spaces? - in case the user accidentally hits the space bar and then q
| } | ||
|
|
||
| async run(): Promise<void> { | ||
| await this.logout() | ||
| } | ||
|
|
||
| async logout(tip = true): Promise<void> { | ||
| async logout(showInstructions = true): Promise<void> { |
src/services/logout-service.ts
Outdated
| import {CliUx} from '@oclif/core' | ||
| import {LogService} from '../services/log-service' | ||
| import {AbortService} from '../services/abort-service' | ||
| import axios, { AxiosRequestConfig } from 'axios' |
There was a problem hiding this comment.
all these extra spaces, can you remove those back to how things were - to keep the same style. i know this is getting nitpicky, but let's do that until ts-standard gets added to the repo.
| // 404 - keep trying | ||
| await CliUx.ux.wait(2000) // check every 2 seconds | ||
| await this.check(tip) // check again | ||
| } else if (this.checkCount < LogoutService.MAX_CHECK_COUNT) { |
There was a problem hiding this comment.
small but very nice improvement. MAX_CHECK_COUNT 💛
