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

Bump GitHub package #186

Merged
merged 6 commits into from
Dec 11, 2022
Merged

Bump GitHub package #186

merged 6 commits into from
Dec 11, 2022

Conversation

mauricioszabo
Copy link
Contributor

No description provided.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

This looks great to me and is an important update. Love seeing the ChangeLog being updated,

But otherwise do we have to keep the 0.36.12-fix? Looks like a regular tag was also generated on github

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Hope that this new version improves things, which it seems that it is at least slightly. Just gotta keep the rest of it up to date with these new changes, then I say lets merge

@benonymus
Copy link
Contributor

Is anything blocking this pr in particular?

@confused-Techie
Copy link
Member

Is anything blocking this pr in particular?

So this PR is essentially valid, but due to having to technically bump twice over the course of this PR, some of the other details are inaccurate as you can see in my review comments.

Suppose we haven't gotten around to fixing those yet, but I may just commit my comments myself, and take a look at the tests how things are going.

I think the PR lost some steam since it didn't totally fix the issues we saw, but still wouldn't be bad to at least include another bump in here

@benonymus
Copy link
Contributor

Is anything blocking this pr in particular?

So this PR is essentially valid, but due to having to technically bump twice over the course of this PR, some of the other details are inaccurate as you can see in my review comments.

Suppose we haven't gotten around to fixing those yet, but I may just commit my comments myself, and take a look at the tests how things are going.

I think the PR lost some steam since it didn't totally fix the issues we saw, but still wouldn't be bad to at least include another bump in here

What does it not fix?

As far as I remember the only reason for the bump was to get to a version that supports apple silicon.
Which this version should do.
I have been using the apple silicon build form here: #124
for the last month every day without any issue.

@confused-Techie
Copy link
Member

What does it not fix?

Performance Issues mainly, but yeah fair enough it does tackle the main reason it was created. I'll go ahead and apply the reviews manually, and merge this one.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

The tests here look good, and like previously stated this adds support for Apple Silicon, I'll go ahead and merge

@confused-Techie confused-Techie merged commit 69c2c21 into master Dec 11, 2022
@Spiker985 Spiker985 deleted the bump-github-package branch February 24, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants