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

Include namespaces #21

Merged
merged 9 commits into from
Oct 11, 2017
Merged

Include namespaces #21

merged 9 commits into from
Oct 11, 2017

Conversation

nesaulov
Copy link
Owner

#14

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.462% when pulling e376d82 on include_namespaces into a02642a on master.

Repository owner deleted a comment from coveralls Oct 10, 2017
Repository owner deleted a comment from coveralls Oct 10, 2017
Repository owner deleted a comment from coveralls Oct 10, 2017
Repository owner deleted a comment from coveralls Oct 10, 2017
@nesaulov
Copy link
Owner Author

@AlessandroMinali what do you think?

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.1%) to 99.485% when pulling f44c61e on include_namespaces into 6147808 on master.

Copy link
Collaborator

@AlessandroMinali AlessandroMinali left a comment

Choose a reason for hiding this comment

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

Comprehensive specs 👍 Looks good besides my one nitpick on the error

This does not have anything to do directly with this but something that came into my head while reading the changes/code: I wonder if there is a way which can greatly reduce the amount of copy and pasting of params between methods. Maybe building an object/Struct when the API is called to store the opts and calling the inner methods on that so we don't need to keep juggling things around. Something to think about and probably better tackled in the other refactor issue

#
# @return [Hash] a nested hash.
def break_namespaces(klass, camelize:, nesting_level:)
Surrealist::ExceptionRaiser.raise_invalid_nesting_level! if nesting_level.zero?
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about error on less than zero and not a Integer

@coveralls
Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage increased (+0.1%) to 99.49% when pulling 5a250d9 on include_namespaces into 6147808 on master.

Repository owner deleted a comment from coveralls Oct 11, 2017
Repository owner deleted a comment from coveralls Oct 11, 2017
Repository owner deleted a comment from coveralls Oct 11, 2017
Repository owner deleted a comment from coveralls Oct 11, 2017
@coveralls
Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage decreased (-0.4%) to 98.98% when pulling 4917bba on include_namespaces into 6147808 on master.

@nesaulov
Copy link
Owner Author

nesaulov commented Oct 11, 2017

@AlessandroMinali yep, you are right, I've also thought about it and I guess this needs to be done. We already have an issue for Surrealist::Builder refactoring (#8), and @tegon is (I guess) working on it. But it's definitely true that we need some struct to carry data across methods, I'll create an issue now.
Also I have just realised that it would be better to have some parameters sanity check before any action, I mean if someone passed iclude_root: 'please no', it would be checked during the initial #surrealize method invocation, not inside the guts like StringUtils or Copier. I'll work on this today.

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.

3 participants