Use Hash composition over inheritance

[Sija]

Inheriting from stdlib classes is considered a bad practice (it's my personal opinion, but there's at least crystal-lang/crystal#3238 (comment) suggesting I'm not alone here).

[ysbaddaden]

I see little, if any, benefit for this change. Maybe changing Dependency and Target to have strict ivars (if it's possible) would probably be better.

[straight-shoota]

Yes, I think this would be a proper improvement @ysbaddaden 👍

[ysbaddaden]

Yet, that's still nitpicking until #1 and #184 are implemented for example.

[Sija]

I don't agree it's nitpicking, and even if, that's still better than the status quo. Also, I dunno what does it have to do with other issues you mentioned. Of course it would be great to have them fixed, but why to block minor improvements over sth that has been waiting for to be done for 3 years now?

[Sija]

@ysbaddaden I'd appreciate if you'd engage in any sort of merit-based conversion before closing PRs. Also, playing down even small, but still incremental improvements along with naming things like nitpicking and pedantic creates unwelcoming atmoshpere for all people involved.

This change for instance was seen as positive by several people already (2 PRs with exact same change were accepted, third was closed due to refactoring which took care of the root problem) so it would be nice if you could tone-down your powerplay a bit...

[ysbaddaden]

I apologize if it sounds harsh. I just don't want someone to spend much time on such changes, and prefer to close early —hinting at issues that are worth spending time on for developers and reviewers.

These classes are Hashes on purpose (they may have any kind of key/values); they do work without any issue. Legacy code accumulates, but there are much deeper changes required to make Shards a real package manager that will (in)directly involve these classes —maybe they'll be structs or just be removed.

I could shrug and merge —which I did for single vs double quotes— but this change is just opinion, doesn't change anything in my point-of-view, and let's spend time and energy on fixing issues that will require deep changes in the code base instead.

[Sija]

Hi @ysbaddaden!

Because of the current design decisions, shards used a library trggers Crystal ICE in certain conditions, therefore makes it impossible to use it alongside other shards (or at least one I know of - raven.cr), see for instance recent comment by @straight-shoota - crystal-lang/crystal#6253 (comment).

Would you mind reconsidering this PR as a valid fix for the above problem - and not a nitpicking, like you've previously stated?

Best regards

[Sija]

@ysbaddaden 🏓

[ysbaddaden]

I have zero time to spend on crystal related stuff :(

I don't like the forward missing methods, it's proof the design is bad.

A real change would make Dependency and Target use explicit attributes, with custom yaml pull parsers, instead of behaving as a Hash.