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

Misleading claim on readme: administrate does have a DSL. #2267

Open
rafaelsales opened this issue Sep 22, 2022 · 5 comments · May be fixed by #2505
Open

Misleading claim on readme: administrate does have a DSL. #2267

rafaelsales opened this issue Sep 22, 2022 · 5 comments · May be fixed by #2505

Comments

@rafaelsales
Copy link

rafaelsales commented Sep 22, 2022

No DSLs (domain-specific languages)

A DSL is more than class methods invoked in class definitions.

Are we really declaring that the following is not a DSL:

ATTRIBUTE_TYPES = {
  status: Field::Select.with_options(
    searchable: false,
    collection: %w[draft review approved] },
  )
}

and the following would be a DSL?

attribute_type :status, Field::Select.with_options(
  searchable: false,
  collection: %w[draft review approved] },
)

What is so great about using constants vs class methods? That is still a set of predefined names that the user has to know and follow their expected value structure in order to work, so IMO there's absolutely no difference.

I believe that the greatest disadvantage of ActiveAdmin was that they went too far with the DSL: they created a DSL even for defining forms when you could just write a form erb file using the Rails helpers with not many more lines of code.

But to me, having to define things in a declarative way via constants with specific names in Administrate is not different at all from calling class methods on the class definition such as ActiveAdmin does for declaring basic things such as columns available for the index page.

@pablobm
Copy link
Collaborator

pablobm commented Oct 6, 2022

Yeah, I agree. I have been thinking that for a while, and even have felt at times that the constants can be a bit annoying and would rather have them as methods. (Although that won't happen for now, as there are other priorities).

Would you be able to volunteer a rephrasing of that portion of the README?

@nickcharlton
Copy link
Member

Yeah, I agree too. I think if we were to re-approach the problem now class methods would be a nicer way of doing things — I definitely avoid constants where I can usually.

It'd be great to re-phrase in such a way to demonstrate the difference in approach you mention — we want it to be easy to override and get out of the way and that's the main motivation of the project.

@c4lliope
Copy link
Contributor

c4lliope commented Nov 7, 2022

Seems unanimous, I like your example of ActiveAdmin's approach, they had been my main inspiration for changing our old approach. Ended up seeing a couple 1500-line AA dashboard files once, makes you shudder.

So I hope some clear direction can be gained.
I like some examples I'm seeing online, mainly led by ActiveRecord, RSpec years ago -
chained options as methods on classes.

ATTRIBUTE_TYPES = {
  status: Field::Select.with_options(
    searchable: false,
    collection: %w[draft review approved] },
  )
}

... could become ...

def self.build
  shape(:status) { Field::Select.searchable(false).collection(%w[draft review approved]) }
end

@azeemh

This comment was marked as off-topic.

nickcharlton added a commit that referenced this issue Feb 2, 2024
The original intent of the line in the README was to emphasise that it's
not fully DSL-driven, instead as close to standard Rails as possible.

This rephrases to avoid saying "we don't have a DSL", but try and
provide a more useful way to describe the projects' vision.

Closes #2267
@nickcharlton nickcharlton linked a pull request Feb 2, 2024 that will close this issue
@nickcharlton
Copy link
Member

I'm gradually building towards releasing v1.0.0 and this has been on my mind as I've been trying to think about what that might look like.

I've opened #2505 to try and remove the DSL suggestion and come up with a better wording.

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 a pull request may close this issue.

5 participants