already initialized constant SKIP_ATTRIBUTES in resource_spec #228

Closed
tvdeyen opened this Issue Apr 26, 2012 · 7 comments

Comments

Projects
None yet
3 participants
Owner

tvdeyen commented Apr 26, 2012

alchemy_cms/spec/libraries/resource_spec.rb:95: warning: already initialized constant SKIP_ATTRIBUTES

Shouldn't you use an attr_accessor instead of a constant?

@ghost ghost assigned masche842 Apr 26, 2012

Contributor

masche842 commented Apr 26, 2012

Yes, maybe that's better. I thought: The developer decides once which columns to show and which not. But as there is a default value, a constant doesn't make much sense. I'll refactor when looking at the failing spec mentioned in #229

@tvdeyen tvdeyen closed this in ebb30f5 May 4, 2012

Contributor

masche842 commented May 15, 2012

I now tried to adjust an existing app for the new way of skipping attributes. Old way was easy: Just declare SKIP_ATTRIBUTES in your model and you're done. But I can't figure out how to do that easily the new way.
Do you have any ideas? I mean, probably we are not done yet with this. Maybe it's better to relay on a class method defined in the model (self.skip_attributes)?

Owner

tvdeyen commented May 15, 2012

Yes, a class method, or an entry in Alchemy's config.yml file.

Example:

´´´

config.yml

skip_model_attributes: [foo, bar, widget]
´´´

´´´

somehwere in your resources code

Alchemy::Config.get(:skip_model_attributes)
´´´

best
t

Am 15.05.2012 um 12:29 schrieb Marc:

I now tried to adjust an existing app for the new way of skipping attributes. Old way was easy: Just declare SKIP_ATTRIBUTES in your model and you're done. But I can't figure out how to do that easily the new way.
Do you have any ideas? I mean, probably we are not done yet with this. Maybe it's better to relay on a class method defined in the model (self.skip_attributes)?


Reply to this email directly or view it on GitHub:
magiclabs#228 (comment)

Contributor

masche842 commented Jun 22, 2012

Ok, I implemented it as an optional class-method in the model. I think to decide which attributes are visible and, as a consequence, are changeable belongs to domain logic and therefore belongs there.
Drawback: It's harder to test because one has to undefine the class-method afterwards...

Then I coincidently figured out you implemented resource_relations as configuration-option. I don't like that really, because things are scattered all over the place... But it works and is already implemented. So what do you think, should I code the skip_attributes like resource_relations?

Owner

tvdeyen commented Jun 22, 2012

@robinboening implemented the 'resource_relations'. Please ask him.

Owner

robinboening commented Jun 22, 2012

Adjusting the attribute´s visibility in the model also feels cleaner for me.
I also dont like to scatter things on different places that actually should stick together.
I agree with you.

In my opinion we should implement the skip_attributes feature in the way you did now.
But we should then refactor the resource_relations feature, so that we have a common way of configuration.

@masche842 and @tvdeyen do you agree with me?

Contributor

masche842 commented Jun 24, 2012

I agree. Hopefully there aren't too many people that are using the new api already...

I'll try to implement both this week. As I said, testing is a bit, mmh, brittle, maybe we can find a solution for that toegether.

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