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

Better validation errors #195

Merged
merged 2 commits into from
Oct 22, 2019
Merged

Better validation errors #195

merged 2 commits into from
Oct 22, 2019

Conversation

fisx
Copy link
Collaborator

@fisx fisx commented Jun 15, 2019

Fixes #140

Stolen from servant-swagger

@fisx
Copy link
Collaborator Author

fisx commented Jun 15, 2019

thanks @sphaso!

@fisx
Copy link
Collaborator Author

fisx commented Jun 22, 2019

any comments on this? it would be nice to get this merged, since it also improves on the error messages provided in servant-swagger slightly (it also dumps the descriptions on the swagger docs).

thanks!

@fisx
Copy link
Collaborator Author

fisx commented Jul 8, 2019

ping...

swagger2.cabal Outdated
@@ -62,6 +62,7 @@ library
-- GHC boot libraries
build-depends:
base >=4.7 && <4.13
, aeson-pretty
Copy link
Collaborator

Choose a reason for hiding this comment

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

bounds

@fisx
Copy link
Collaborator Author

fisx commented Jul 29, 2019

just a friendly reminder that i'm still interested in this. @phadej? or perhaps somebody else has the authority to decide that i've answered all his requests?

@fisx
Copy link
Collaborator Author

fisx commented Sep 28, 2019

Under what conditions would it be possible for me to become a co-maintainer? I am interested in using this library professionally, but I would have to either fork it or make it somehow easier for changes to happen here.

@fizruk
Copy link
Member

fizruk commented Sep 28, 2019

@fisx Apologies for late responses!

Right now I don't have enough time lately to properly maintain this package (I have all the functionality I need for my projects, so maintaining is low priority). Apparently, @phadej also has his plate full. So I think we would only benefit from with adding you as a maintainer here and, probably, on Hackage 😊

@phadej
Copy link
Collaborator

phadej commented Sep 28, 2019

There's also a conflict...

@phadej
Copy link
Collaborator

phadej commented Sep 28, 2019

FYI: I dislike comments with commit message Fixup, so if @fizruk gives @fisx a maintainer bits, we'd need to agree a bit more rules as there would be more cooks in the kitchen (i.e. write them down).

@fisx
Copy link
Collaborator Author

fisx commented Sep 28, 2019

FYI: I dislike comments with commit message Fixup, so if @fizruk gives @fisx a maintainer bits, we'd need to agree a bit more rules as there would be more cooks in the kitchen (i.e. write them down).

I am used to squashing during merge, so I'm sometimes not too concerned with commit messages. If you don't want to allow squash-merge in this repo, I would like to go through my branch again and clean it up a little.

In general, of course I'm willing to follow your rules if you are nice enough to let me work with you (even though I'll argue :)). I suggest we write down whatever comes to mind, and I'll leave all PRs open a minimum amount of time to give you time to add more rules as the need arises?

Thanks for the fast reply (this time)! :)

@fisx fisx requested a review from phadej September 29, 2019 15:13
@fisx
Copy link
Collaborator Author

fisx commented Sep 29, 2019

Thanks for inviting me to mess with this project, I will try very hard not to disappoint you! :-)

I will let this PR sit for your final approval / more change requests for another 5 days. If I don't hear from you, I will assume your approval, merge, and make a github release. I would prefer though to get a nod from you @fizruk and @phadej on this policy in general, at least this time.

And I'm happy to do the hackage release as well if you let me in on that, too. About that: I have tried to guess the way versions work in this project in the last commit: it's a non-breaking change, but it's not just a revision either, so 2.4 becomes 2.4.1. Does that make sense to everybody?

@fizruk
Copy link
Member

fizruk commented Sep 30, 2019

@fisx I would replace 5 days with 2 weeks, but the rule seems okay to me otherwise.

Regarding versioning: we are sticking with PVP for this package, so act accordingly. Either way, release process (including version bump and changelog updates) takes a separate PR, so don't do it here.

GetShop.TV coding style is available here. For commit messages please refer to a blog post by Chris Beams on «How to Write a Good Commit Message». I am typically not in favour of squash merge, but I do not have strong opinions against it.

Meanwhile I will try add more people from @GetShopTV to join in as co-maintainers to make it easier to get things reviewed.

P.S. Can you write me at nickolay at getshoptv dot com to discuss other details?

@fisx
Copy link
Collaborator Author

fisx commented Oct 8, 2019

@fisx I would replace 5 days with 2 weeks, but the rule seems okay to me otherwise.

Ok, two weeks start now. I will merge either on Oct 22, or any time before that if I have your approval.

Regarding versioning: we are sticking with PVP for this package, so act accordingly. Either way, release process (including version bump and changelog updates) takes a separate PR, so don't do it here.

Ok, I have removed the commit.

GetShop.TV coding style is available here. For commit messages please refer to a blog post by Chris Beams on «How to Write a Good Commit Message». I am typically not in favour of squash merge, but I do not have strong opinions against it.

I like both articles a lot, thanks! I think I've done ok before even knowing them, but I've found some things I haven't thought of so far that I want to incorporate into my style.

Meanwhile I will try add more people from @GetShopTV to join in as co-maintainers to make it easier to get things reviewed.

👍

P.S. Can you write me at nickolay at getshoptv dot com to discuss other details?

done!

thanks :)

@fisx fisx mentioned this pull request Oct 11, 2019
@fisx fisx merged commit db0b871 into GetShopTV:master Oct 22, 2019
@fizruk fizruk mentioned this pull request Nov 23, 2019
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.

Validation errors don't provide any context
3 participants