-
Notifications
You must be signed in to change notification settings - Fork 95
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
Optional dataType #140
Optional dataType #140
Conversation
SwiftLint found issues
Generated by 🚫 Danger Swift against 3b0bd4e |
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.
Great job, I love how you went over all code and updated the documentation as well. For a second I figured we should validate and potentially assert to have a dataType if the status code is not matching an empty response, but I figured we would take too much responsibility. I like the flexibility we give right now!
I've got one nit, and our repository requires signed commits. Would it be possible for you to enable signed commits and recommit using the following command:
git rebase --exec 'git commit --amend --no-edit -n -S' master
git push --force
You can read more about setting up signed commits here.
b6132a2
to
bea1bf8
Compare
The commits are now signed. :-) |
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 stuff, thanks @chkpnt!
@chkpnt thanks for signing the commits, you're almost there! Can you finish that as well? 🙂 |
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.
It would be amazing if we could make this a non-breaking change. Let me know if that's something I can help with. :)
Now we have 7 public initializers, which all need to be maintained... 🫣 I guess it's time to introduce a builder. (probably out of scope of this PR?) |
🙈 Yeah, I would say that is out of scope, but an interesting idea! |
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.
Lookin' gooooood. Thanks so much for this, @chkpnt 🤩
A few small things I think we might want to tackle, and then I think this is good to go 👌
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.
Thanks a lot for the time and effort you put into making this PR shine! 💪
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.
💯
Congratulations! 🎉 This was released as part of Release 3.0.2 🚀 Generated by GitBuddy |
As discussed in #134
I've also added an additional initializer to simplify the mock creation in case a single non-get request has to be mocked.