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

Enhance status badge for PRs in [GithubIssueDetail] #3295

Merged
merged 1 commit into from Apr 14, 2019

Conversation

Projects
None yet
3 participants
@ice1000
Copy link
Contributor

commented Apr 13, 2019

I'm basically following other PRs -- I am not an expert on JS.
Please don't hesitate to spot any problems.

This PR addresses #1719

@shields-ci

This comment has been minimized.

Copy link

commented Apr 13, 2019

Messages
📖 Thanks for your contribution to Shields, @ice1000!

Generated by 🚫 dangerJS against da23009

@ice1000 ice1000 force-pushed the ice1k:master branch from 6265867 to b0b0963 Apr 13, 2019

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

There are some linter errors that I don't quite understand:

  53:27  error  Identifier 'merged_at' is not in camel case  camelcase
  53:38  error  Identifier 'closed_at' is not in camel case  camelcase
  55:9   error  Identifier 'merged_at' is not in camel case  camelcase
  57:9   error  Expected property shorthand                  object-shorthand
  61:16  error  Identifier 'closed_at' is not in camel case  camelcase
  63:9   error  Expected property shorthand                  object-shorthand
  69:9   error  Expected property shorthand                  object-shorthand
  77:13  error  Identifier 'merged_at' is not in camel case  camelcase
  77:24  error  Identifier 'closed_at' is not in camel case  camelcase

What if the API is using snake case?

@ice1000 ice1000 force-pushed the ice1k:master branch from b0b0963 to e4018f4 Apr 13, 2019

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Trying to use this code:

    return this.constructor.render({
      number,
      mergedAt: json['merged_at'],
      closedAt: json['closed_at'],
    })

instead of matching the properties out directly. Hope the CI will be happy with that.

@ice1000 ice1000 force-pushed the ice1k:master branch 2 times, most recently from 89f63f8 to 8804ff9 Apr 13, 2019

@calebcartwright
Copy link
Member

left a comment

Thanks for this!

However, I'm not convinced this is the right approach to address #1719. As I mentioned on that thread, the code has changed a lot, and I think those changes make some alternative solutions worth considering first.

Ultimately the ask behind #1719 is for slightly enhanced rendering logic (the badge message and color) on the existing GH Issue Detail state badge when the referenced item is a PR. I think creating an entirely separate service class with an additional badge route has the potential to create confusion (see below) and adds duplicative code that will have to be maintained.

With the approach implemented here in this PR, we'd end up with two separate badge routes and service classes that functionally only differ in the badge message and coloring, but exposed via two entirely separate paths:

The current/existing route:
https://img.shields.io/github/issues/detail/state/badges/shields/3269.svg

And then we'd also have
https://img.shields.io/github/status/merge-status/badges/shields/3269.svg

I also think it would be a confusing user experience to have one badge route pattern (/github/issues/detail/...) that would have to be used for all other GH detail badges (labels, author, title, comments, update, etc.) but if the target item is a PR then the user would have to use a different path

IMO we could probably address #1719 by tweaking the logic in GithubIssueDetail service for the respective transform and render functions

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

What I am thinking about is that the badge rendering logic is completely different and they're using different GitHub APIs.

Several questions:

  • Is it a good idea to have 2 API requests in the issue detail badge logic?
  • Isn't the badge rendering logic I've added are quite different from existing ones?

Yes, these questions are all present in the original issue, but my reason of adding a new badge is that I don't want to have 2 API requests in one badge and the logic is pretty different (because we're using different APIs!).

@calebcartwright

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Is it a good idea to have 2 API requests in the issue detail badge logic?

No one is proposing making 2 sequential API calls. There's plenty of badge service implementations that use the values of route/query parameters to conditionally determine the URL call to make, the schema to use, how to transform the response, and how to render.

Many such conditions are already handled within the existing service class.

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

route/query parameters to conditionally determine the URL call to make

This is very very reasonable! I've got a plan in mind, please comment:

  • Add a new which to the issue detail service, like pull
  • If the which is pull, use the logic that I've implemented to determine the message
@calebcartwright

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

This is very very reasonable! I've got a plan in mind, please comment:

  • Add a new which to the issue detail service, like pull
  • If the which is pull, use the logic that I've implemented to determine the message

That's probably the approach I'd suggest as well. Below are some snippets from what I was mocking up locally, may be a useful reference:

Update the state map:
https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail.service.js#L21

const stateMap = {
  schema: Joi.object({
    ...commonSchemaFields,
    state: Joi.string()
      .allow('open', 'closed')
      .required(),
    merged_at: Joi.string().allow(null),
  }).required(),
  transform: ({ json }) => ({
    state: json.state,
    merged: json.merged_at !== null,
  }),
  render: ({ value, isPR, number }) => {
    const state = value.state
    const label = `${isPR ? 'pull request' : 'issue'} ${number}`

    if (!isPR || state === 'open') {
      return {
        color: stateColor(state),
        label,
        message: state,
      }
    }

    if (value.merged) {
      return {
        label,
        message: 'merged',
        color: 'blueviolet',
      }
    }

    return {
      label,
      message: 'rejected',
      color: 'red',
    }
  },
}

the service class route:

  static get route() {
    return {
      base: 'github',
      pattern:
        ':kind(issues|pulls)/detail/:which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
    }
  }

the service class example will need to have the namedParams and staticPreview tweaked:

...
        namedParams: {
          kind: 'issues',
          which: 'state',
          user: 'badges',
          repo: 'shields',
          number: '979',
        },
        staticPreview: this.render({
          which: 'state',
          value: { state: 'closed' },
          isPR: false,
          number: '979',
        }),
...

and the service implementations for fetch, handle and transform at the bottom of the file:

  async fetch({ kind, which, user, repo, number }) {
    return this._requestJson({
      url: `/repos/${user}/${repo}/${kind}/${number}`,
      schema: whichMap[which].schema,
      errorMessages: errorMessagesFor('issue, pull request or repo not found'),
    })
  }

  transform({ json, which, kind }) {
    const value = whichMap[which].transform({ json, which })
    const isPR = json.hasOwnProperty('pull_request') || kind === 'pulls'
    return { value, isPR }
  }

  async handle({ kind, which, user, repo, number }) {
    const json = await this.fetch({ kind, which, user, repo, number })
    const { value, isPR } = this.transform({ json, which, kind })
    return this.constructor.render({ which, value, isPR, number })
  }

And lastly, the redirector that's currently remapping pulls to issues here: https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.service.js#L22 will need to be removed

@calebcartwright

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

There's a few tests that would need to be updated as well:

@ice1000 ice1000 force-pushed the ice1k:master branch 3 times, most recently from 802923b to a211023 Apr 13, 2019

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Done, please review.

@ice1000 ice1000 force-pushed the ice1k:master branch from a211023 to 222434b Apr 13, 2019

@calebcartwright

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Excellent! I'll review here shortly

@calebcartwright calebcartwright changed the title Add pull request merge/open/closed badge Enhance status badge for PRs in [GithubIssueDetail] Apr 13, 2019

@ice1000 ice1000 force-pushed the ice1k:master branch 3 times, most recently from 7c58c2f to 8bb31d6 Apr 13, 2019

@ice1000 ice1000 force-pushed the ice1k:master branch 2 times, most recently from 9d1716d to 6a41601 Apr 14, 2019

@ice1000

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Addressed all comments, rebased on master, please take a look.

@ice1000 ice1000 force-pushed the ice1k:master branch 5 times, most recently from e44d590 to 306b496 Apr 14, 2019

@ice1000 ice1000 force-pushed the ice1k:master branch from 306b496 to da23009 Apr 14, 2019

@calebcartwright

This comment has been minimized.

@calebcartwright
Copy link
Member

left a comment

Nice!

@calebcartwright calebcartwright merged commit e12e625 into badges:master Apr 14, 2019

9 checks passed

LGTM analysis: JavaScript No new or fixed alerts
Details
ci/circleci: danger Your tests passed on CircleCI!
Details
ci/circleci: e2e Your tests passed on CircleCI!
Details
ci/circleci: frontend Your tests passed on CircleCI!
Details
ci/circleci: integration@node-latest Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: main@node-latest Your tests passed on CircleCI!
Details
ci/circleci: services Your tests passed on CircleCI!
Details
ci/circleci: services@node-latest Your tests passed on CircleCI!
Details
@shields-deployment

This comment has been minimized.

Copy link

commented Apr 14, 2019

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.