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

fix: trim nextLink before slicing #309

Merged
merged 1 commit into from
Sep 7, 2022
Merged

fix: trim nextLink before slicing #309

merged 1 commit into from
Sep 7, 2022

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Apr 29, 2022

Hey all! I think I found and fixed a small bug; using this PR to start a discussion. When I tried updating Redwood's all contributors table, I got a Only absolute URLs are supported error, so I dug in a bit. This is the command I ran:

yarn all-contributors check --config .all-contributorsrc

And here's the .all-contributorsrc file: https://github.com/redwoodjs/redwood/blob/main/tasks/all-contributors/.all-contributorsrc.

I stepped through the code and narrowed it down; the error's happening in the getNextLink function. Here's the value for link:

'<https://api.github.com/repositories/191051391/contributors?per_page=100&page=1>; rel="prev", <https://api.github.com/repositories/191051391/contributors?per_page=100&page=3>; rel="next", <https://api.github.com/repositories/191051391/contributors?per_page=100&page=3>; rel="last", <https://api.github.com/repositories/191051391/contributors?per_page=100&page=1>; rel="first"'

The function splits link on commas and looks for rel=next. In this case, it's the second link. So splitting it results in:

// Note the space
' <https://api.github.com/repositories/191051391/contributors?per_page=100&page=3>; rel="next"'

That means the next call (return nextLink.split(';')[0].slice(1, -1);) results in:

// We got rid of the space, but < remains
'<https://api.github.com/repositories/191051391/contributors?per_page=100&page=3'

When another function tries to fetch this link, it throws.

It seems like all we need to do is trim the string before slicing, but let me know what you think!

@jtoar jtoar changed the title Trim nextLink before slicing Trim nextLink before slicing Apr 29, 2022
Copy link
Member

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

That looks good to me. Could you check the pipeline and fix the issue that is happening?

@jtoar
Copy link
Contributor Author

jtoar commented Aug 27, 2022

Hey @Berkmann18, I can't seem to access the logs of the failing check. Do I need permissions or is there another way I can see what's going on there?

@Berkmann18
Copy link
Member

@jtoar It 404'ed for me, so I re-ran the failing jobs.

@tenshiAMD tenshiAMD changed the title Trim nextLink before slicing refactor: trim nextLink before slicing Sep 7, 2022
@tenshiAMD tenshiAMD enabled auto-merge (squash) September 7, 2022 08:25
@tenshiAMD tenshiAMD changed the title refactor: trim nextLink before slicing fix: trim nextLink before slicing Sep 7, 2022
@tenshiAMD tenshiAMD disabled auto-merge September 7, 2022 08:30
@tenshiAMD tenshiAMD enabled auto-merge (squash) September 7, 2022 08:30
@tenshiAMD tenshiAMD merged commit 5df6b47 into all-contributors:master Sep 7, 2022
@all-contributors-release-bot
Copy link
Member

🎉 This PR is included in version 6.20.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

tenshiAMD added a commit that referenced this pull request Sep 13, 2022
* origin/master: (85 commits)
  refactor: log full error stack on error (#316)
  chore: fix status badges (#315)
  docs: add JoshuaKGoldberg as a contributor for bug (#314)
  fix: incorrect usage of `tbody` (#311)
  fix: trim `nextLink` before slicing (#309)
  fix: set default value as `7` for `contributorsPerLine` (#139)
  chore(deps): bump dependencies and devDeps (#298)
  refactor: add tbody to contributors table (#307)
  docs: add Lucas-C as a contributor for doc (#306)
  fix: scriptName + improving usage messages (#305)
  docs: add vapurrmaid as a contributor (#304)
  chore(deps): CVE-2021-23337 in inquirer->lodash (#303)
  docs: add SirWindfield as a contributor (#297)
  feat: add namespaced token (#296)
  docs: add LaChapeliere as a contributor (#292)
  feat(contribution-types): add research contribution type (#291)
  docs: add darekkay as a contributor (#290)
  feat: display a meaningful error when the config file is missing (#288)
  docs: add melink14 as a contributor (#285)
  docs: add jdalrymple as a contributor (#264)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants