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

Refactor internal API #25

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Refactor internal API #25

merged 3 commits into from
Sep 22, 2023

Conversation

jwoertink
Copy link
Member

@jwoertink jwoertink commented Sep 22, 2023

Fixes #23

This is a pretty big refactor and breaking change that moves the internal methods from instance methods to class methods.

This changes up quite a few things.

  • Reverts the task_name macro back to name like it was before, but keeps the generated method as task_name
  • Changes the generated summary name to task_summary
  • Summary is now optional
  • Changes the help_message to task_help_message
  • Moves all of these from instance methods to class methods

Because Crystal treats macros like class methods, and arguments on macros are always optional, you can't really have a class method with no args be the same name as a macro. So to avoid a compilation error, the methods are named different from their macro counterparts.

Old interface

class My::Task < LuckyTask::Task
  name "new.name"
  summary "Here is my description"

  def help_message
    "my custom help message"
  end

  def call
    # do stuff
  end
end

task = My::Task.new
task.name #=> "new.name"
task.summary #=> "Here is my description"
task.help_message #=> "my custom help message"
task.print_help_or_call(["-v"])

New Interface

class My::Task < LuckyTask::Task
  name "new.name"
  summary "Here is my description"
  help_message "my custom help message"

  def call
    # do stuff
  end
end

My::Task.task_name #=> "new.name"
My::Task.task_summary #=> "Here is my description"
My::Task.task_help_message #=> "my custom help message"
My::Task.new.print_help_or_call(["-v"])

@jwoertink jwoertink marked this pull request as ready for review September 22, 2023 01:43
@jwoertink jwoertink added the BREAKING CHANGE This breaks backwards compatibility label Sep 22, 2023
@matthewmcgarvey
Copy link
Member

The issue you saw with trying to use a class function that has the same name as a macro, I wonder why that doesn't affect anything when the macro is name. Seems like an import class method that core Crystal would call on the class in cases like errors and stuff.

Copy link
Contributor

@mdwagner mdwagner left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'm not seeing any glaring issues.

@jwoertink
Copy link
Member Author

@matthewmcgarvey it turns out that using name does have the same affect if the name macro also generates a class method name, but it doesn't have that affect when it does anything else... I tried to see how Crystal handles that, but it looks like they may make special considerations for that https://github.com/crystal-lang/crystal/blob/fb92a1e94669fd736c40d7b93611f43d5db5e028/spec/std/class_spec.cr#L71-L74

@jwoertink
Copy link
Member Author

Pulled this in to Lucky and everything seems to run ok. 1 weird issue that came up was we have this method https://github.com/luckyframework/lucky/blob/ecb57b1e8f104567b9615eee3b29ca8345753cb0/tasks/routes.cr#L61-L68 which gets called both in the instance level output and the help message... With the help message moving to a class method, these aren't as "shareable"... Still doable, but just a little moving around.

Otherwise it all feels fine

@jwoertink jwoertink merged commit 1acdb48 into main Sep 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This breaks backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider an even newer name for task_name
3 participants