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: return unique repositories only #90

Merged
merged 2 commits into from
Sep 29, 2022
Merged

fix: return unique repositories only #90

merged 2 commits into from
Sep 29, 2022

Conversation

stefanbuck
Copy link
Member

I've noticed that Octoherd runs multiple times on the same repository even the code was written in a way to actual prevent this. The current implementation doesn't work, because resolvedRepositories is an array of objects like

 {
    id: 243966911,
    node_id: 'MDEwOlJlcG9zaXRvcnkyNDM5NjY5MTE=',
    name: 'OctoLinker',
    full_name: 'OctoLinker/OctoLinker',
   ....
}

but the current implementation can deal with primitive types like number , string , boolean only.

Before

npx @octoherd/script-hello-world -R 'OctoLinker/OctoLinker' -R 'OctoLinker/OctoLinker'

INFO  Running on OctoLinker/OctoLinker ...
INFO  Hello, World! From OctoLinker/OctoLinker
INFO  Running on OctoLinker/OctoLinker ...
INFO  Hello, World! From OctoLinker/OctoLinker

After

npx @octoherd/script-hello-world -R 'OctoLinker/OctoLinker' -R 'OctoLinker/OctoLinker'

INFO  Running on OctoLinker/OctoLinker ...
INFO  Hello, World! From OctoLinker/OctoLinker

// return array with unique repositories based by id (https://stackoverflow.com/a/56757215)
return resolvedRepositories.filter((repo, index, repoList) => repoList.findIndex(v2 => (v2.id === repo.id)) === index);
// return array with unique repositories based by name (https://stackoverflow.com/a/56757215)
return resolvedRepositories.filter((repo, index, repoList) => repoList.findIndex(v2 => (v2.name === repo.name)) === index);
Copy link
Member Author

@stefanbuck stefanbuck Sep 29, 2022

Choose a reason for hiding this comment

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

I changed id to name to make the test passing. However, I think we should use eitherfull_name or id here to avoid situations where repo names are the same in different organisations. Doing so will result in a slightly bigger refactoring (mainly around tests) so wanted to discuss it first. What do you think @gr2m

Copy link
Member

@gr2m gr2m Sep 29, 2022

Choose a reason for hiding this comment

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

I'd definitely prefer to compare by id, can you do a follow up issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, no problem

@gr2m gr2m merged commit 30dac18 into main Sep 29, 2022
@gr2m gr2m deleted the fix-unique-repos branch September 29, 2022 23:56
@github-actions
Copy link

🎉 This PR is included in version 3.4.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants