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

a typo often ends up creating structs that then make other names brea… #112

Closed
wants to merge 1 commit into from

Conversation

@grosser
Copy link
Contributor

grosser commented Dec 14, 2015

…k ... adding a sanity check to make these errors easier to spot

@chrisroberts

…k ... adding a sanity check to make these errors easier to spot
@grosser grosser force-pushed the grosser:grosser/missing branch from ed2bb37 to 183c470 Dec 14, 2015
@chrisroberts

This comment has been minimized.

Copy link
Member

chrisroberts commented Dec 17, 2015

The idea is sound but the implementation would break expected functionality as no struct values would be allowed to be passed within the configuration hash. Would be better to type check on the specific items that are cared about, using the expected types. Raising a type error would be a better fit as well. Will use this as a starting point and make adjustments. Thanks!

@grosser

This comment has been minimized.

Copy link
Contributor Author

grosser commented Dec 17, 2015

I don't expect any meaningful / good solution to be 100% compatible ... we are using this now, which works pretty well ... does not allow chaining like foo.bar.baz (can be rewritten as nesting) but blocks all kinds of method missing calls since they usually look like:

foobar insert_typo_here
# calling methods without arguments or blocks smells like a method missing
::SparkleFormation::SparkleStruct.prepend(Module.new do
  def method_missing(name, *args, &block)
    caller = ::Kernel.caller.first

    called_without_args = (
      args.empty? &&
      !block &&
      caller.start_with?(File.dirname(File.dirname(__FILE__))) &&
      !caller.include?("vendor/bundle")
    )
    internal_method = (name =~ /^_|\!$/)

    if called_without_args || internal_method
      message = "undefined local variable or method `#{name}` (use block helpers if this was not a typo)"
      ::Kernel.raise NameError, message
    end
    super
  end
end)
@grosser grosser closed this Dec 17, 2015
@grosser grosser deleted the grosser:grosser/missing branch Dec 17, 2015
@grosser

This comment has been minimized.

Copy link
Contributor Author

grosser commented Apr 27, 2016

FYI switched to allowing only uppercase methods

  • easy to see what is a method and what is an attribute
  • closer to original cloudformation / easier to paste
  • no camel-case bugs
# make our templates only support know keys
::SparkleFormation::SparkleStruct.prepend(Module.new do
  def method_missing(name, *args, &block)
    # using upcase attribute names is fine / cannot be confused with method calls
    return super if name =~ /^[A-Z]/

    # ok for sfn itself / any other gem
    return super if ::Kernel.caller.first.include?('/gems/')

    # not ok for us to use bad names
    ::Kernel.raise NameError, "undefined local variable or method `#{name}`"
  end
end)
@chrisroberts

This comment has been minimized.

Copy link
Member

chrisroberts commented Apr 27, 2016

If this works for your specific use case then that's great. For the library at large this introduces multiple breaking changes as well as reduces the functionality of the DSL itself:

  • Project specific naming conventions can aid in easy identification of "attributes"
  • CloudFormation is not the only supported template format
  • SparkleFormation itself is aimed at reducing copy/paste not enabling it
  • Explicit sets work fine for handling non-conventional casing styles
  • Restricting to only capitalized methods breaks method chaining

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.