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

Axiom types refactor #36

Merged
merged 3 commits into from
Aug 6, 2013
Merged

Axiom types refactor #36

merged 3 commits into from
Aug 6, 2013

Conversation

dkubb
Copy link
Owner

@dkubb dkubb commented Jun 27, 2013

This branch will include a refactoring of axiom to use axiom-types.

  • Remove unnecessary DEFAULT_SIZE constants since the types use the defaults from axiom-types anyway
  • Move static #type methods to become class methods, and have #type delegate to it
  • Remove Axiom::Attribute::Date#range
  • Remove Axiom::Attribute::DateTime#range
  • Remove Axiom::Attribute::Time#range
  • Rename options to Axiom::Attribute::String constructor to be :minimum_length and :maximum_length instead of :min_length and :max_length
  • Update Axiom::Attribute::String constructor to only create a custom type only if the :minimum_length or :maximum_length options are specified
  • Update Axiom::Attribute::Numeric constructor to only create a custom type if a :size option is specified
  • Remove the @options ivar from Axiom::Attribute. Descendants should pull what they need from the options and store an explicit ivar
  • Add a method to axiom-types where given two types we can find the most common ancestor, for example: type.common_ancestor(other).
  • Use Axiom::Types::Type#common_ancestor to return the best type from Axiom::Function::Numeric::Binary#type
  • Audit diff to make sure @example YARD docs, and others, were not accidentally removed during the conversion
  • Ready for review
  • Squash commits

# @return [Range<::DateTime>]
#
# @api public
def range
Copy link
Owner Author

Choose a reason for hiding this comment

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

A #range method needs to be added back to this class like in Axiom::Attribute::Date.

@dkubb
Copy link
Owner Author

dkubb commented Jul 19, 2013

@cored ok I think I have something working that passes the tests.

There's some performance issues with this though, the specs run much slower than before and I think it's in Attribute.infer_type, which I left a comment on.

I also think we should audit the Attribute subclasses and make sure our quick patches didn't miss anything. Ideally we want to remove as much as possible from each Attribute subclass, and delegate things to the type, when possible. I've already left comments hinting in this direction, but I'm sure there's more we can do.

I am going to do a pass over the diff and look at everything we did to make sure there's no extraneous stuff, and make sure it's optimal. Feel free to do the same if you want, the more of us looking at this the better.

#
# @return [Range<::Date>]
#
# @api public
def range
DEFAULT_RANGE
type = self.type
type.minimum..type.maximum
Copy link
Owner Author

Choose a reason for hiding this comment

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

I wonder if maybe instead of #range we can simply delegate #minimum and #maximum to the type?

@dkubb
Copy link
Owner Author

dkubb commented Jul 26, 2013

@cored I just added the --warnings flag to rspec and saw a ton of warnings in axiom-types. I will get those fixed up today, but I wanted to give you the heads up so you didn't think it was something you did.

@dkubb
Copy link
Owner Author

dkubb commented Jul 27, 2013

@cored on the TODO list, I have some notes about removing #range and delegating #minimum and #maximum to the type. I wonder if those are even necessary? Maybe we should just start by removing the #range method and not worry about delegating those other methods until a concrete need appears? wdyt?

@dkubb
Copy link
Owner Author

dkubb commented Jul 27, 2013

@cored I also noticed that we have things like #min_length in String, and it only seems to be used for equalizer's benefit. If we changed Axiom::Attribute itself to equalize on the name, type and required?, then we could eliminate these methods.

Previously all these methods were needed in Attribute because it was handling constraints and validations. Now all that is handled by axiom-types. Maybe someday we'll need a way to get to the type constraints through the attribute, but I would rather remove code until that is totally required.

@dkubb
Copy link
Owner Author

dkubb commented Jul 29, 2013

@cored after thinking about this for a while, I think it is probably unnecessary to completely eliminate the Axiom::Attribute subclasses. I think anything we do to create an intermediary structure to link together methods and types will end up being far more complex than Axiom::Attribute subclasses.

However, I do think we should continue with removing functionality that is duplicated between them. Please note this does not change anything we've done up to this point, and does not affect anything still to be done in this PR. All it means is that once the todo notes in this PR are complete and this code is merged into master, we're essentially done with the integration.

cored and others added 3 commits August 5, 2013 23:04
Signed-off-by: Dan Kubb <dan.kubb@gmail.com>
* This is a temporary change until mutant allows me to exclude
  the Axiom::Types namespace from the classes it mutation tests.
dkubb added a commit that referenced this pull request Aug 6, 2013
Refactor attributes to use axiom-types
@dkubb dkubb merged commit 7a8a7e0 into master Aug 6, 2013
@dkubb dkubb deleted the axiom-types-refactor branch August 6, 2013 06:21
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.

None yet

3 participants