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

Allow migrating legacy services to services/ affects [continuousphp shippable] #1931

Merged
merged 4 commits into from
Aug 22, 2018
Merged

Allow migrating legacy services to services/ affects [continuousphp shippable] #1931

merged 4 commits into from
Aug 22, 2018

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Aug 17, 2018

This change would allow us to break up both server.js and all-badge-examples.json. After rewriting a few of these, I'm convinced the rewriting process would be more pleasant and less error-prone if we break it up into pieces. Porting the badge examples is particularly error prone, since the way they're specified is changing. I think it will be more efficient, and more correct, to port whole categories at once rather than one service at a time. When rewriting a service, I use the old implementation as a reference, and having it on hand in the service folder will make that easier. It'll also let us harmonize the code style; we've been putting off updating server.js. Finally, we'll get more helpful production stack traces which have the service name in them, rather than a server.js line number which is only valid for the specific commit, and thus time consuming to track down.

We seem very close to accepting new services as new-style services, though if we end up taking a couple in this form, it wouldn't be the end of the world.

If folks are cool with this approach, I'll update the developer docs.

Here's the process for moving a service over:

  1. Paste the boilerplate header:
'use strict'

const LegacyService = require('../legacy-service')
const { makeBadgeData: getBadgeData } = require('../../lib/badge-data')
const { checkErrorResponse } = require('../../lib/error-helper')

module.exports = class ___ extends LegacyService {
  static registerLegacyRouteHandler({ camp, cache }) {
  1. Paste in the service code from server.js and add two closing braces.

  2. Run prettier and npm run lint -- --fix.

  3. Add any additional helper imports that may be needed.

I have included one example with the examples migrated, and one without them migrated, to show that both setups are possible. I'd suggest we move over the badge implementations first, and then take a second pass through with the examples. If we move all the implementations at once, it will be really easy to dig past the single commit in server.js to see the blame info for each implementation.

Todo post merge:

  • Update developer docs
  • Migrate badge implementations from server.js
  • Unify code style in project root with the rest of the project
  • Migrate badge examples

@paulmelnikow paulmelnikow added service-badge Accepted and actionable changes, features, and bugs core Server, BaseService, GitHub auth labels Aug 17, 2018
@shields-ci
Copy link

Warnings
⚠️

This PR modified the server but none of the service tests. That's okay so long as it's refactoring existing code.

⚠️

This PR modified service code for continuousphp but not its test code. That's okay so long as it's refactoring existing code.

⚠️

This PR modified service code for shippable but not its test code. That's okay so long as it's refactoring existing code.

Messages
📖

✨ Thanks for your contribution to Shields, @paulmelnikow!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

@paulmelnikow paulmelnikow changed the title Allow migrating legacy services to services/ Allow migrating legacy services to services/ affects [continuousphp shippable] Aug 17, 2018

// registerFn: ({ camp, cache }) => { camp.route(/.../, cache(...)) }
class LegacyService extends BaseService {
static registerLegacyRouteHandler({ camp, cache }) {
Copy link
Member Author

Choose a reason for hiding this comment

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

By making this a dictionary, we can easily inject more collaborators from server.js via register(), if that turns out to be necessary.

@chris48s
Copy link
Member

After rewriting a few of these, I'm convinced the rewriting process would be more pleasant and less error-prone if we break it up into pieces. Porting the badge examples is particularly error prone, since the way they're specified is changing

I haven't found that so much, but if it can be done quickly I can definitely see advantages to getting the bulk of legacy code (or at least the stuff that has tests) into a more consistent structure like this:

  • It would help with the work we're doing to re-organise examples - that one huge data structure for all the examples is a real pain to edit if a lot of stuff needs to change.
  • It would also make it easier to experement with stuff like organising examples in a less rigid way
  • It would make it easier to do stuff like setting variable cache length based on categories because we can attach meta-data to legacy services

Where do you see this fitting in to the wider refactoring process?

The code itself is good - just wondering more about..the plan for using it :)

@paulmelnikow
Copy link
Member Author

Sounds like we've had slightly different experiences, though both think there are good reasons to move forward with this.

Where do you see this fitting in to the wider refactoring process?

I'm thinking we can empty server.js of badge implementations in a matter of an hour or two. It's a little scary to do that on services without tests, though we could manually test, old skool-like, via the frontend listing on the review app.

Migrating the badge examples manually would be a bear, and error prone. I'm guessing we should at least partially automate that process, like I did when originally built that file in #1163. I think moving the services should come first anyway; it's less risky to move the legacy services while the examples are staying the same.

@paulmelnikow paulmelnikow merged commit 942466a into badges:master Aug 22, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status:

@paulmelnikow paulmelnikow deleted the legacy-service branch August 22, 2018 03:38
paulmelnikow added a commit that referenced this pull request Aug 27, 2018
This builds on the work of #1931 by moving the legacy services into `services/`.
paulmelnikow added a commit that referenced this pull request Oct 30, 2018
all-badge-examples is a common cause of merge conflicts. It’s difficult to adjust the badge categorization in that file – or to understand the diff – because it requires moving a block from one point to another. It’s much easier to edit a badge’s category in one place.

This starts the process of breaking up what’s left of that file, following up on the work from #1931.
paulmelnikow added a commit that referenced this pull request Oct 31, 2018
all-badge-examples is a common cause of merge conflicts. It’s difficult to adjust the badge categorization in that file – or to understand the diff – because it requires moving a block from one point to another. It’s much easier to edit a badge’s category in one place.

This starts the process of breaking up what’s left of that file, following up on the work from #1931. New-style services can only be in one category, which means legacy service examples have to be split along category lines. I split out separate legacy service classes where I could do so easily, leaving behind the ones which require more work, for one reason or another.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants