grammyjs / commands

Work-in-progress plugin for managing commands with grammY.
https://grammy.dev
MIT License
12 stars 2 forks source link

feat: add mixin of commands on ctx #15

Closed carafelix closed 5 months ago

carafelix commented 6 months ago

Summary

PD: I closed the other PR since it was a little messy on the branch managment end, this one is ok. PD2: I can refactor this into a MixedCommands class that extends the Commands Class if that suits more the style. Another way would be to add this as a method to the Commands Class, but I don't know good of an idea would be to mix them before toSingleScopeArgs in a method like commands.mergeWith(otherCommands) because it would effectively duplicate commands registration if someone use it on let say commands A and B, and register both into bot.use, registering all at once on A (B was merged into A), and then re-register B commands. Even in that case they would simply overwrite themselves I think so, no?

eg:

pr webm

snippet from the gif (localization in the admin command... 🥴)

// localized start command //
userCommands.command("end", "end", async (c) => {
  await c.setMyCommands(userCommands)
}).localize("es", "fin", "finaliza")
  .addToScope(
    { type: "all_private_chats" },
    async (ctx) => {
      ctx.reply(`finalizado`)
      await ctx.setMyCommands(userCommands)
    },
  );
adminCommands.command("admin", "i am the admin", async (c) => {
  await c.setMyCommands(userCommands, adminCommands);
}).localize("es", "start", "el admin soy yo");
codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 71.42857% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 79.88%. Comparing base (63bc0fa) to head (2c2934f). Report is 2 commits behind head on main.

:exclamation: Current head 2c2934f differs from pull request most recent head 74a2f5a

Please upload reports for the commit 74a2f5a to get more accurate results.

Files Patch % Lines
src/context.ts 65.71% 12 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #15 +/- ## =========================================== + Coverage 68.13% 79.88% +11.74% =========================================== Files 7 7 Lines 408 502 +94 Branches 66 78 +12 =========================================== + Hits 278 401 +123 + Misses 128 100 -28 + Partials 2 1 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

carafelix commented 6 months ago

If the merge commands function is not to clear on what it's doing, just let me know, I can refactor it and can add it as a commands method

carafelix commented 5 months ago

Removed pollution from commit history

carafelix commented 5 months ago

Test added for merging

carafelix commented 5 months ago

Is something missing for this to be merge? I can work on #16 if you think its appropriated

roziscoding commented 5 months ago

image GitHub won't let me merge for some reason and I haven't had the time to do a local merge. Could you try do rebase this branch on the main branch of the official repo and do a push --force-with-lease, please?

carafelix commented 5 months ago

Now it should be good. Merging main into this branch resulted in duplicate commits, since I merged the fixLocalization branch into this one before. Got a little messy along the way, but it's clean and okay now. (I re-checked that it's working on the telegram client)