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

[meme] Split data.rb into multiple files per class #482

Merged
merged 4 commits into from
Nov 11, 2018
Merged

[meme] Split data.rb into multiple files per class #482

merged 4 commits into from
Nov 11, 2018

Conversation

z64
Copy link
Collaborator

@z64 z64 commented Apr 17, 2018

WIP

Not much to say here, but this isn't done quite yet.

In data.rb, there are several utility classes / mixins I decided to leave behind. These are more "helpers" than "data objects". So we have a few choices: leave them there, split them into files under lib/discordrb/, or move them into lib/discordrb.rb together. I would prefer the last I think. Once we decide, I will move the requires into more proper places.

I also elected to keep some classes and modules logically together. (Embed* objects, etc)


TODO:

  • Decide what to do with helpers
  • Split data specs into files to match

Closes #390

@z64 z64 added the code-style label Apr 17, 2018
@z64 z64 added this to the 4.0.0 milestone Apr 17, 2018
@Daniel-Worrall
Copy link
Contributor

+4k -4k memes aside...

If we're moving all these into /data/file.rb, then we should be re-modularising them so that they're inside a Discordrb::Data module. This is to keep a consistent module/class file structure that many other projects stick to.

This is going to need to be integration test spec'd and a lot of require checkings done. Also, I would request this move into a 4.0 branch, not master, so this can be done longer term.

As for what was left in data.rb, I would very much like to see everything top level be moved into discordrb.rb. The rest of it IDObject, PermissionCalculator, I propose go into a Mixin file lib/discordrb/mixin.rb. Also, ColourRGB should be either in discordrb/data/colourrgb.rb or discordrb/colourrgb.rb

As for the groups you proposed, I would prefer seeing them go in their own files. A file for each class. This is a very common project structure and I think discordrb can really benefit from something like this.

@Daniel-Worrall Daniel-Worrall mentioned this pull request Apr 17, 2018
17 tasks
@z64
Copy link
Collaborator Author

z64 commented Apr 18, 2018

Discordrb::Data

No, I don't want such a large breaking change just because of the code scaffolding. While I know namespace-by-path is a practice, I don't think it makes sense for us. I want this merged "quickly" to improve developer QOL above all. On that note;

4.0 branch

I don't see any reason why this can't be master. I have no idea what the 3.3 -> 4.0 timescale is going to be, and there will certainly be PRs from others in the community to 3.3 code in the meantime. Maintaining both branches, handling the merge later, telling people to PR against 4.0, as well as any PRs aimed at master right now will have to be closed and reopened to point at 4.0 (you can't change target branch) - all sounds like headaches I don't want to deal with.

I have no problem with master being unstable, it should be expected. People relying on master should be watching the repo and/or pinning to commits. And again, I don't want to hold back doing this just because of infrastructure things that don't affect the end user (specs/integration).

Let's "move fast and break things"

IDObject/Permission calculator in mixin.rb

Eh, I don't see why modules should get special treatment. They should be in discordrb.rb or their own files I think. Or I wouldn't be opposed either to moving IDObject to discordrb.rb and PermissionsCalculator to permissions.rb with the other permissions code.

Anyway, mixin.rb just feels arbitrary I guess, but I'll do it if no stronger consensus comes up.

Everything else (helper methods/constants) that remains moving to discordrb.rb sounds good to me.

As for the groups you proposed, I would prefer seeing them go in their own files.

I thought about this, but like all of these objects are just straight JSON-mapped objects. They have tiny/zero behavior footprint and will always be considered with the rest of their counterpart classes.

@z64
Copy link
Collaborator Author

z64 commented Apr 18, 2018

Also a thought I just had

If we don't like

  • Embed
  • EmbedAuthor
  • EmbedFooter
    etc.

I would be perfectly okay with

  • Embed
  • Embed::Author
  • Embed::Footer
    etc.
class Embed
  class Author
  end

  class Footer
  end
end

We do this in other places, and this is very unlikely to be a breaking change that affects anyone as these are read-only classes that referencing them directly doesn't have many use cases.

@z64 z64 mentioned this pull request May 18, 2018
@z64
Copy link
Collaborator Author

z64 commented Sep 4, 2018

Some updates on this in prep for the next release 🎉

  • Rebased onto the latest master
  • IDObject and ColourRGB now have their own homes
  • PermissionsCalculator has been moved into permissions.rb

(I realized we already had a precedent for mixins: cache.rb!)

Now data.rb is entirely a "prelude" for our abstraction layer.

I think I'd like to leave EmbedXYZ alone for now. If we want to change the namespacing, we can do so later.

z64 added 4 commits November 6, 2018 14:02
Move utility methods to discordrb.rb

Move IDObject to id_object.rb

Move PermissionsCalculator to permissions.rb

Move ColourRGB to colour_rgb.rb
@z64 z64 merged commit 4af31f1 into discordrb:master Nov 11, 2018
@z64 z64 deleted the data branch November 11, 2018 08:27
@z64 z64 mentioned this pull request Dec 10, 2018
z64 added a commit that referenced this pull request Dec 10, 2018
Follow up to #482. I created new files accordingly but looks like I
forgot to remove the relevant specs from `data_spec.rb`.
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.

2 participants