search

voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.52k stars 242 forks source link

Extracted all limits out to their own file #342

Closed iainbeeston closed 7 years ago

iainbeeston commented 8 years ago

Right now all limit attributes are defined as nested classes of Limit. We've already done this for Format classes. Moving them out to their own classes reduces bulk and makes it easier to find the class you need.

RST-J commented 7 years ago

I see that this is intended to be only a refactoring in terms of moving classes around. We can just merge this. But actually I don't like the class hierarchy. A MaxFoo class that is inheriting from the MinFoo class solely because it sets the right data type is a code smell to me. Wouldn't it be worth to restructure things a bit? Having a StringLimit class which sets the the string data type and then let inherit both, Min and Max variants from it? I mean, this is arguably a comparably minor thing and we have bigger monoliths to decompose, but I thought its worth a consideration. What do you think?

iainbeeston commented 7 years ago

Thanks @RST-J that's a really good point. I've refactored out a few parent classes as you suggested.

I've also added another commit, to pull out attribute classes that were nested in the file of another attribute into their own file.

Could you please tell me what you think and if I should make further changes?

RST-J commented 7 years ago

I'll have a look as soon as I find some time and hopefully can give you feedback in a couple of days.

RST-J commented 7 years ago

I think its good to go 👍