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

Add method to get an array of types a module is directly included in #8133

Merged
merged 6 commits into from Dec 7, 2019

Conversation

@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Aug 31, 2019

module Foo
end

module Baz
  module Tar
    include Baz
  end
end

abstract class Parent
end

class Bar < Parent
  include Foo
  include Baz
end

struct Str
  include Baz
end

struct Gen(T)
  include Baz
end

abstract struct AStr
  include Baz
end

abstract class ACla
  include Baz
end

class SubT(T)
  include Baz
end

class ChildT(T) < SubT(T)
end

{{Baz.all_includers.map &.name.stringify}} # => ["Baz::Tar", "Bar", "Str", "Gen(T)", "AStr", "ACla", "SubT(T)"]

Questions:

  • How do I test this?
  • Are there any other types that can include a module i should try?
  • Any other ideas for a better name?

I also noticed that types with generics include the generic type vars in their name. I would have thought Gen(T) would have just been Gen...not sure if this is intended or not.

EDIT: I added it to macros.cr, will include that in feedback fixes.

@kalinon

This comment has been minimized.

Copy link
Contributor

kalinon commented Aug 31, 2019

Awesome. this will be quite useful if merged. I cant think of anything off the top of my head that also includes modules.

@sam0x17

This comment has been minimized.

Copy link
Contributor

sam0x17 commented Aug 31, 2019

idea: you could make a macro that generates a bunch of arbitrary modules to test

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Aug 31, 2019

Any use case for this?

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Aug 31, 2019

Mainly as a compliment to #subclasses. ATM the only way to get an array of types is using that. This allows for using modules to achieve a similar effect if inheritance isn't the best solution.

Ref https://forum.crystal-lang.org/t/type-all-includers/254

EDIT: I suppose all_includers might be a better name to match all_subclasses

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Sep 10, 2019

How well does this work with querying generic modules? I'd like this method in but @asterite should sign off on this being complete,

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Sep 10, 2019

@RX14 Seems to work fine.

module Bar(T)
end

module Foo
  include Bar(String)
end

module Baz
  include Bar(Bool)
end

{{Bar.all_includers.map &.name.stringify}} # => ["Foo", "Baz"]
@asterite

This comment has been minimized.

Copy link
Member

asterite commented Sep 10, 2019

I think the main issue is what happens if a module is included by a generic type. It's probably the same issue we have with subclasses. That's why I'm not sure how and whether this should be added to the macro methods.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Sep 10, 2019

module Bar(T)
end

class Test(T)
  include Bar(T)
end

{{Bar.all_includers.map &.name.stringify}} # => ["Test(T)"]

Is what seems to happen. Same thing as #subclasses.

@kalinon

This comment has been minimized.

Copy link
Contributor

kalinon commented Sep 10, 2019

module Bar(T)
end

class Test(T)
  include Bar(T)
end

{{Bar.all_includers.map &.name.stringify}} # => ["Test(T)"]

Is what seems to happen. Same thing as #subclasses.

I think this is acceptable behavior.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Oct 31, 2019

Anything left to do here?

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Nov 25, 2019

Bump, this should be good to review.

Blacksmoke16 and others added 5 commits Aug 31, 2019
Rename to all_includers
Rename internal method to match macro method
Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
@Blacksmoke16 Blacksmoke16 force-pushed the Blacksmoke16:included-macro-method branch from 854e92d to c03d27a Nov 25, 2019
@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Nov 25, 2019

Also FWIW, #8483 will allow you to skip the generic type vars of the module (for both this and #subclasses/#all_subclasses (ref: #8133 (comment)))

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Nov 28, 2019

@asterite @RX14 @bcardiff Anyone able to do another review of this? Would be 💯 if this could make it into 0.32.0.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 5, 2019

I want ary or brian to sign off on this.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Dec 5, 2019

After working with this locally a bit I'm starting to think I should rename it to #includers, since this only returns types that directly include the module, not children of those types.

I'll push that tonight.

Since this only includes types the module is directly included in
@Blacksmoke16 Blacksmoke16 changed the title Add method to get an array of types a module is included in Add method to get an array of types a module is directly included in Dec 5, 2019
@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

Blacksmoke16 commented Dec 7, 2019

Copy link
Member

asterite left a comment

We are already dancing, so let's keep doing it

@asterite asterite merged commit 4e9c44b into crystal-lang:master Dec 7, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite added this to the 0.32.0 milestone Dec 7, 2019
@Blacksmoke16 Blacksmoke16 deleted the Blacksmoke16:included-macro-method branch Dec 7, 2019
@Blacksmoke16 Blacksmoke16 mentioned this pull request Dec 16, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.