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

some cosmetics for Metadata class #523

Merged
merged 1 commit into from Dec 5, 2011

Conversation

Projects
None yet
2 participants
@AlexKVal
Contributor

AlexKVal commented Dec 4, 2011

(tested 1.8.7 & 1.9.2)

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 4, 2011

Member

I need to do some research on this. The two different definitions of full_description_for were there to fix an issue. I see that your changes don't fail any existing specs, but I want to go back and make sure that issue is covered.

Member

dchelimsky commented Dec 4, 2011

I need to do some research on this. The two different definitions of full_description_for were there to fix an issue. I see that your changes don't fail any existing specs, but I want to go back and make sure that issue is covered.

@AlexKVal

This comment has been minimized.

Show comment
Hide comment
@AlexKVal

AlexKVal Dec 4, 2011

Contributor

It seems to me the Metadata class needs some refactoring. IMHO.
But I've got no such knowledge and experience to do such changes.

Contributor

AlexKVal commented Dec 4, 2011

It seems to me the Metadata class needs some refactoring. IMHO.
But I've got no such knowledge and experience to do such changes.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 4, 2011

Member

What specifically makes you feel like this class needs refactoring? I agree that it does, but I wonder if our motivations are the same.

Member

dchelimsky commented Dec 4, 2011

What specifically makes you feel like this class needs refactoring? I agree that it does, but I wonder if our motivations are the same.

@AlexKVal

This comment has been minimized.

Show comment
Hide comment
@AlexKVal

AlexKVal Dec 4, 2011

Contributor

If RESERVED_KEYS would convert into class methods, it would be no reason for them to be reserved.
The method #process actually is kind of Fabric-method for ExampleGroup metadata, but not completely.
It returns Metadata object instead of ExampleGroup metadata.
Same for the method #for_example - it must be used only in form @example_group_metadata.for_example (and it does),
but it is possible to misunderstand and do something like Metadata.new.for_example. But I think it's not big deal with #for_example when look at it in isolated context (without #process for example).
There are so much "self[:example_group]" through the project, I think it would be nice to convert it to method,
(and with using store(:example_group, value) - may be gain some performance :) )
I would be try to make a variant with classes for Example and ExampleGroup metadata maybe.
And I think there are some logic duplications in "parents / ancestors" determination in ExampleGroup #superclass_metadata and Metadata, all those store(:example_group, {:example_group => parent_group_metadata[:example_group]}.
Phh.. Sorry for my english I've got lack of practice to write. :(
But I'm trying very hard :)

Contributor

AlexKVal commented Dec 4, 2011

If RESERVED_KEYS would convert into class methods, it would be no reason for them to be reserved.
The method #process actually is kind of Fabric-method for ExampleGroup metadata, but not completely.
It returns Metadata object instead of ExampleGroup metadata.
Same for the method #for_example - it must be used only in form @example_group_metadata.for_example (and it does),
but it is possible to misunderstand and do something like Metadata.new.for_example. But I think it's not big deal with #for_example when look at it in isolated context (without #process for example).
There are so much "self[:example_group]" through the project, I think it would be nice to convert it to method,
(and with using store(:example_group, value) - may be gain some performance :) )
I would be try to make a variant with classes for Example and ExampleGroup metadata maybe.
And I think there are some logic duplications in "parents / ancestors" determination in ExampleGroup #superclass_metadata and Metadata, all those store(:example_group, {:example_group => parent_group_metadata[:example_group]}.
Phh.. Sorry for my english I've got lack of practice to write. :(
But I'm trying very hard :)

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 5, 2011

Member

That's a lot in one comment :) I won't respond to all at once, but let's start w/ RESERVED_KEYS.
I agree that some of them should be methods, but I think I'd still want most of them to be reserved just to minimize confusion.

I'll follow up later on the other issues.

Member

dchelimsky commented Dec 5, 2011

That's a lot in one comment :) I won't respond to all at once, but let's start w/ RESERVED_KEYS.
I agree that some of them should be methods, but I think I'd still want most of them to be reserved just to minimize confusion.

I'll follow up later on the other issues.

dchelimsky added a commit that referenced this pull request Dec 5, 2011

Merge pull request #523 from AlexKVal/meta
some cosmetics for Metadata class

@dchelimsky dchelimsky merged commit 45eb0d3 into rspec:master Dec 5, 2011

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Dec 5, 2011

Member

I found the spec for the bug I was worried about and it passed after these changes, so I've merged them.

Member

dchelimsky commented Dec 5, 2011

I found the spec for the bug I was worried about and it passed after these changes, so I've merged them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment