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(modules): create display, qr, directory modules #1152

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

LoneRifle
Copy link
Contributor

@LoneRifle LoneRifle commented Feb 1, 2021

Problem

The infrastructure to generate QR codes is poorly organised and too dependent on
calling inversify to resolve its dependencies. This makes it inconsistent in structure
relative to the other controllers and modules in Go.

Whilst we are here, we should move RotatingLinksController into its own module

Part of ongoing work for #869

Solution

  • Create display module, move RotatingLinksController into it
  • Create qr module, move QrCodeService and assets into it
  • Extract QrCodeController from api/qr into modules/qr
  • move GoDirectory to module
  • Inject config, existing inversify dependencies into classes
    wherever possible

- Create display module, move RotatingLinksController into it
- Create qr module, move QrCodeService and assets into it
- Extract QrCodeController from `api/qr` into `modules/qr`
- Inject config, existing inversify dependencies into classes
  wherever possible
@LoneRifle LoneRifle force-pushed the refactor/modules/misc branch from c72ec36 to 842fb60 Compare February 2, 2021 11:46
@LoneRifle LoneRifle changed the title refactor(modules): create display, qr modules refactor(modules): create display, qr, directory modules Feb 2, 2021
@LoneRifle LoneRifle force-pushed the refactor/modules/misc branch from 269554c to 9deb8e9 Compare February 3, 2021 03:19
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

Agreed that the qrcode route was exposing too much logic and should be refactored into its own controller. Most of the code lgtm; i'm raising one discussion point to ponder about before changes go in.

src/server/modules/qr/QrCodeController.ts Show resolved Hide resolved
@LoneRifle LoneRifle requested a review from yong-jie February 8, 2021 02:14
@LoneRifle LoneRifle merged commit 345ce90 into develop Feb 8, 2021
@LoneRifle LoneRifle deleted the refactor/modules/misc branch February 8, 2021 04:17
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.

2 participants