AlchemyCMS / alchemy_cms

Alchemy is the Open Source Rails CMS framework for the component based web that can be used as classic server side rendered or headless CMS.
https://www.alchemy-cms.com
BSD 3-Clause "New" or "Revised" License
839 stars 315 forks source link

already initialized constant SKIP_ATTRIBUTES in resource_spec #228

Closed tvdeyen closed 12 years ago

tvdeyen commented 12 years ago

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?

masche842 commented 12 years ago

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

masche842 commented 12 years ago

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)?

tvdeyen commented 12 years ago

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: https://github.com/magiclabs/alchemy_cms/issues/228#issuecomment-5712845

masche842 commented 12 years ago

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?

tvdeyen commented 12 years ago

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

robinboening commented 12 years ago

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?

masche842 commented 12 years ago

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.