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

Added header for apps #823

Merged
merged 10 commits into from Nov 1, 2019
Merged

Added header for apps #823

merged 10 commits into from Nov 1, 2019

Conversation

AAllport
Copy link
Contributor

Copy link
Collaborator

@acrobat acrobat left a comment

Choose a reason for hiding this comment

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

Can you add the line to set the header to each api method instead of the configure method, this way it's easier to remove it when the beta period is over (example). You could make the configure method private and call it in each api method for example. Thanks!

@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

Certainly!

@AAllport AAllport requested a review from acrobat November 1, 2019 15:54
@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

@acrobat is there a release schedule?
Unfortunately, my pr on GrahamCampbell/Laravel-GitHub#95 was rejected, so would be nice to know when we can hope to see these changes hit packagist?

@acrobat
Copy link
Collaborator

acrobat commented Nov 1, 2019

@AAllport There is no planned release schedule, but I'm planning on publishing a new release soon as there are many fixed/features since last release!

@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

@AAllport There is no planned release schedule, but I'm planning on publishing a new release soon as there are many fixed/features since last release!

Looking forward to it!

@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

Well that's ruddy mysterious

@acrobat
Copy link
Collaborator

acrobat commented Nov 1, 2019

@AAllport the travis error is not related to these changes but something incorrectly merged on master, I've pushed a fix to the master branch. Can you rebase your branch onto master? Thanks!

lib/Github/Api/Apps.php Outdated Show resolved Hide resolved
@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

My git foo isn't quite as good as some be.
If I do a git pull upstream master and it merges, will that suffice (where upstream is this repo)?
IIRC it will merge commit. or did you specifically want a rebase?

@acrobat
Copy link
Collaborator

acrobat commented Nov 1, 2019

@AAllport A merge is fine! I will squash the commits when merging the pr!

@acrobat
Copy link
Collaborator

acrobat commented Nov 1, 2019

Thanks a lot @AAllport! And congrats on your first contribution! 🎉

@acrobat acrobat merged commit 5071f3e into KnpLabs:master Nov 1, 2019
@acrobat acrobat removed the needs-work label Nov 1, 2019
@AAllport
Copy link
Contributor Author

AAllport commented Nov 1, 2019

For what it's worth, I believe this is my 1st functional OSS contribution, 10/10 would definitely recommend 😁
I look forward to the release
All the best!

@acrobat
Copy link
Collaborator

acrobat commented Nov 3, 2019

@AAllport I've published 2.12.0!

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.

Accept header not set for Apps API
2 participants