-
-
Notifications
You must be signed in to change notification settings - Fork 205
feat(provider): add support for all-inkl.com (qdm12#245) #309
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there thanks for your PR! Note someone just did a similar PR 2 days before you 😄 So sorry I wasn't quicker at reviewing. Anyway I decided to work on this PR only and drop the other one, although I'll mention @marc in the merged commit.
Just a few small things to resolve and let's get this merged!
I'll also add a 'sample' provider in the code (with latest good practices etc.) so it's easier for people to copy pasta and modify to add providers.
Hey @michaelmiklis do you have time to look into this? Otherwise we'll pick up from the other opened PR, please let me know. Thanks! |
Unfortunately I received the mail notification today, that there is some work to do for me. I will do my best |
Added new provider all-inkl.com
Co-authored-by: Quentin McGaw <[email protected]>
I added the Readme and acceppted all code changes. |
A few problems as mentioned in the comments. Make sure you update your forked master branch with this master branch, they seem to be some conflicts (i.e. he provider is modified 🤔). Also the linting workflow fails, try running |
Co-authored-by: Quentin McGaw <[email protected]>
thx for pointing that out. you're right Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
I fetched your changes from master, commited all your suggestions and fixed the linting error using gofmt -w. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍 Just please remove that .DS_Store
file please 😉 And I'll merge it finally 👍
@@ -0,0 +1,179 @@ | |||
package allinkl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the binary file internal/settings/providers/.DS_Store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!!! Merging!
Added new provider all-inkl.com