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

feat: implement InputMediaBuilder #210

Closed
wants to merge 12 commits into from

Conversation

WingLim
Copy link
Contributor

@WingLim WingLim commented May 16, 2022

Create builders subdirectory which other builders can put themselves in it.

InputMediaBuilder methods:

Basic methods:

  • add
  • build

Advanced methods:

All these methods use Omit to remove type property for each different InputMedia type, so that user can use it more convenient.

  • photo
  • video
  • animation
  • audio
  • document

Static method:

from method to create a InputMediaBuilder with InputMedia.

  • from

Furthermore, it have constructor to initialize with some InputMedia.

It means if you like OOP, you can use new InputMediaBuilder(/** data*/), or you can use InputMediaBuilder.from(/** data*/) if you like FP more.

TODO:

  • Documentation for InputMediaBuilder

Close #101

@WingLim WingLim marked this pull request as draft May 16, 2022 14:57
@WingLim
Copy link
Contributor Author

WingLim commented May 16, 2022

@KnorpelSenf Plz take a look the code, if it's good for you, then I will write the documentation for it.

@KnorpelSenf
Copy link
Member

Thank you so much! I will be able to take a closer look on Wednesday, but perhaps someone else will have a look before me.

Copy link
Member

@rojvv rojvv left a comment

Choose a reason for hiding this comment

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

I recommend changing the file name to something simpler (one word), or to input_media_builder.ts. So it is consistent with either the other grammY files or official Deno libraries.

@WingLim
Copy link
Contributor Author

WingLim commented May 16, 2022

I think we can rename it to media.ts or input_media.ts, because it's in builders directory, so the builder suffix can be removed.

I prefer the second one, it looks more accurate.

@WingLim
Copy link
Contributor Author

WingLim commented May 16, 2022

The static method from comes from #104, and according to #211 (comment), we can use constructor to do this job.

I don't know which one is better?

@WingLim WingLim marked this pull request as ready for review May 19, 2022 07:18
@WingLim
Copy link
Contributor Author

WingLim commented May 19, 2022

The static method from comes from #104, and according to #211 (comment), we can use constructor to do this job.

I don't know which one is better?

I have added them both, in case someone like the other style more.

@KnorpelSenf KnorpelSenf added this to In progress in Coding May 19, 2022
Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

What do you think about also providing something like InputMediaBuilder.Photo and the like? That way, we could create stuff using InputMediaBuilder.Photo.from(...) which can be passed to InputMediaBuilder.from in turn. In addition, it allows for new InputMediaBuilder.Photo() with chained method calls. The main builder could then also support the sub-builders, so that build does not have to be called on all of them.

I am also wondering if there is a better way to create these things than specifying a config object for every entry. Perhaps this is already possible with the idea described above?

src/convenience/builders/input_media.ts Outdated Show resolved Hide resolved
src/convenience/builders/input_media.ts Outdated Show resolved Hide resolved
Coding automation moved this from In progress to Review in progress May 19, 2022
@WingLim
Copy link
Contributor Author

WingLim commented May 20, 2022

I have a idea about this.

Add a fromType static method which accept 2 params:

  1. The media array
  2. The real type of InputMedia

Then use a GenericInputMediaBuilder to constraint the type of the specific InputMedia(e.g. InputMediaPhoto) builder:

// This builder can only add `InputMediaPhoto` object.
const builder = InputMediaBuilder.fromType<InputMediaPhoto>(media, "photo");

Let me show you the code to explain it more.

@KnorpelSenf KnorpelSenf changed the title Implement InputMediaBuilder feat: implement InputMediaBuilder May 29, 2022
@KnorpelSenf
Copy link
Member

Please also add some tests.

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

I now think it is overkill to have a builder for an array. I remember thinking that I would like to abstract that away, but now I believe this would be overkill and take it too far.

I used the rest your changes to create #437. This will subsequently be closed. Thank you anywy for this contribution (and sorry for the long delay)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Coding
Review in progress
Development

Successfully merging this pull request may close these issues.

feat: Media group builder
3 participants