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

Implement class_* accessor macros #3658

Merged
merged 1 commit into from Dec 20, 2016

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Dec 8, 2016

class Object
  class_getter
  class_getter!
  class_getter?
  class_property
  class_property!
  class_property?
  class_setter
end

Along the lines of #444 (comment)

@RX14
Copy link
Contributor

RX14 commented Dec 8, 2016

Personally I would much rather alter the accessor macros to allow property self.foo : Bar etc. I think the syntax is much nicer.

@Sija
Copy link
Contributor Author

Sija commented Dec 8, 2016

@RX14 I'd prefer that too but since #receiver is available only for Def type I'm not sure if it's possible to implement ATM or how to do it. Any ideas?

@RX14
Copy link
Contributor

RX14 commented Dec 8, 2016

Looks like self.foo : Bar isn't actually valid syntax to pass to the macro at the moment. Could we change that @asterite?

@asterite
Copy link
Member

asterite commented Dec 8, 2016

@RX14 I thought about that, but what does it mean by itself?

@RX14
Copy link
Contributor

RX14 commented Dec 9, 2016

@asterite just a TypeDeclaration with an invalid left side. We already use some syntax that lexes but will never compile (unused variable etc.) for macros. I imagine that a TypeDeclaration can specify pretty much anything on the left hand side.

@Sija
Copy link
Contributor Author

Sija commented Dec 10, 2016

@RX14 I still like the conciseness of property self.foo syntax but on the second thought that would leave string/symbol variant of property "foo", :foo without an equivalent, thus bringing IMO inconsistency to this feature. Also, that would be less ruby-like which uses more verbose version (class_attribute in rails for example). Therefore I'd still suggest using proposed solution, especially when there's no need for syntax changes or extra work on the compiler side to make it happen.

@RX14
Copy link
Contributor

RX14 commented Dec 10, 2016

Is there really any reason why the old syntax exists? It's much less flexible in that you can't add type restrictions or defaults.

@Sija
Copy link
Contributor Author

Sija commented Dec 10, 2016

@RX14 what do you mean by "old syntax"?

@RX14
Copy link
Contributor

RX14 commented Dec 10, 2016

:foo and "foo". They have to be constants anyway, so why not just use barewords.

@bcardiff
Copy link
Member

:foo was the first syntax it was supported, then, when some type restrictions were wanted to be supported the idea of using type declarations foo : Foo or a call / bare word foo fit.

I wouldn't mind dropping :foo con consistency.

I understand the motivation of allowing property self.foo. Crystal/ruby use self. prefix for class methods. But class_property seems a bit more declarative to me.

@Sija Sija force-pushed the class-accessor-macros branch 2 times, most recently from 6143ca1 to 1baa5f4 Compare December 11, 2016 15:57
@Sija
Copy link
Contributor Author

Sija commented Dec 12, 2016

@asterite @bcardiff Is this PR GTG for you?

@asterite
Copy link
Member

@Sija I have to review this. Even though I kind of agree with the name, reusing all the logic and having macros inside macros obfuscates the code quite a bit. But I guess it's harmless. I'll review it soon :-)

@Sija
Copy link
Contributor Author

Sija commented Dec 12, 2016

@asterite Sure, even GH doesn't want to display the diff from default ;) I agree it ain't look gr8, but that was the only sensible way I've found to implement this feature without blatant copy-pasting all of the code once moar...

@Sija
Copy link
Contributor Author

Sija commented Dec 16, 2016

@asterite If this is GTG I'd send next PR simplifying macros in question if that's OK with you. It would save some bytes and simplify the code.

@asterite
Copy link
Member

@Sija Don't worry, I'll soon review the code and probably merge it. These macros are almost fundamental and they won't change over time, so it's not that problematic if we use nested macros here.

{%
macro_prefix = prefixes[0]
method_prefix = prefixes[1]
var_prefix = prefixes[2]
Copy link
Member

Choose a reason for hiding this comment

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

@Sija We can do this:

macro_prefix = prefixes[0].id
method_prefix = prefixes[1].id
var_prefix = prefixes[2].id

Then everywhere below we don't need to use .id and code is shorter and clearer (and slightly faster). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gr8! I've pushed changes with your suggestion applied :)

@asterite
Copy link
Member

@Sija I reviewed this and I think it's really good. I'm glad we can use macros inside comments to generate nice docs :-)

I just made a tiny comment for improvement. After that I'll merge this.

@asterite
Copy link
Member

@Sija Thank you! 💙

@asterite asterite merged commit bf5f57b into crystal-lang:master Dec 20, 2016
@asterite asterite added this to the 0.20.2 milestone Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants