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

feat: auto-fetching - continuation of #196 #259

Merged
merged 5 commits into from
May 24, 2020
Merged

feat: auto-fetching - continuation of #196 #259

merged 5 commits into from
May 24, 2020

Conversation

jdalrymple
Copy link
Contributor

@jdalrymple jdalrymple commented Apr 20, 2020

See #196 for the previous discussion

What:

Adds an auto-fetching mechanism accessible via the fetch command (which requires a PRIVATE_TOKEN env. variable to be set) like #186 but with a cleaner PR that isn't too far from the master branch (which made the other PR hard to rebase).

As noted in all-contributors/all-contributors#18, not all 27 categories can be picked from a GH repo alone (cf. #186 for a table listing what it was able to detect so far).

Why:

To resolve #117 and partly all-contributors/all-contributors#18 (TL;DR: auto adding contributors from a repo).
Re mntnr/name-your-contributors#45

How:

Using name-your-contributors and ac-learn.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

resolves #187

@jdalrymple
Copy link
Contributor Author

jdalrymple commented Apr 20, 2020

As a side note, a lot of the dependencies of this package should be updated or dropped like request which is deprecated. Perhaps using rollup as well?

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.

Would it be possible to get this PR to target the dl branch?
This way, the feature could be finished before merging this.
Alternatively, I could update the relevant branch on my fork and get you to be a collaborator there so the feature could be implemented without crucial parts missing (cf. comments I left in the code).

@jdalrymple
Copy link
Contributor Author

Yea ill go through it, ill have to rebase back onto dl. I figured it was a while since it was created so i rebased onto master. Ill revert it back tomorrow.

@jdalrymple
Copy link
Contributor Author

jdalrymple commented Apr 21, 2020

It didnt take as long as i thought, its been rebased! @Berkmann18 Its ready when ever you are.

@jdalrymple
Copy link
Contributor Author

Following up, any updates on this?

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.

@jdalrymple Thanks for the reminder.

A welcomed modernisation with a few errors (see my comments).

Have you tried running this command on an example repo and see if you run into #187 or not?

Thanks for the contributions so far; hopefully, this will hit master soon 😀.

src/discover/learner.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
src/cli.js Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
src/cli.js Show resolved Hide resolved
src/cli.js Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
@Berkmann18 Berkmann18 self-assigned this May 23, 2020
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.

Woo! We're getting there.

src/cli.js Outdated Show resolved Hide resolved
@jdalrymple
Copy link
Contributor Author

Sorry for the missed await!!

@Berkmann18
Copy link
Member

Berkmann18 commented May 24, 2020

@jdalrymple It's ok.
I noticed some issues on the CI that affected several PRs (it's now fixed tho), so this PR would need to be in-sync then it should be good to go once the bug is gone.
At the same time, a new AC-Learn model would need to be created for this feature to work.
I've not got around to updating the playground model since the addition of various new contribution types but I suspect the git history still contains the old learner.json model.

Edit: I've found and re-added the missing model, the only things left are ensuring #187 is fixed and that the feature is at least somewhat usable and this PR is ready to be merged.

@jdalrymple
Copy link
Contributor Author

jdalrymple commented May 24, 2020

The error you mention i never seem to recreate. Initially the issue looked like it was linked to an improper async file read, but i fixed that :/. Can it be recreated in the dl branch?

@Berkmann18
Copy link
Member

Berkmann18 commented May 24, 2020

@jdalrymple Yeah, true.
I'll see if that the issue still persists once this is rebased and merged.

@Berkmann18 Berkmann18 linked an issue May 24, 2020 that may be closed by this pull request
@Berkmann18
Copy link
Member

@jdalrymple I'm not able to fix the conflict which prevents this PR to be merged, could you please have a look?

@Berkmann18 Berkmann18 merged commit cc99369 into all-contributors:dl May 24, 2020
@Berkmann18
Copy link
Member

@all-contributors Please add @jdalrymple for code

@allcontributors
Copy link
Contributor

@Berkmann18

I've put up a pull request to add @jdalrymple! 🎉

@NaffanDroo
Copy link

Hi is this feature still lined up to make the final cut into a release?

@Berkmann18
Copy link
Member

Hi is this feature still lined up to make the final cut into a release?

I resumed work on that, and I've made some improvements and testing changes locally (still work to do but at least it's not on hold).

Berkmann18 added a commit that referenced this pull request Jul 23, 2023
* fix: Adjusting file access that was clashing with uncontrolled promises

* review: Adding code review changing and linting

* fix(cli): rectify an async call

Co-authored-by: Maximilian Berkmann <maxieberkmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetch or auto-discover contributors (auto-generate) Adding contributors wreck
3 participants