RealSpeaker / telegraf-session-local

Telegraf local sessions middleware with multiple supported storage types (Memory/FileSync/FileAsync/...) using lowdb
https://git.io/v7iw9
MIT License
93 stars 10 forks source link

Typo in the full example #152

Closed Taoufik-B closed 3 years ago

Taoufik-B commented 3 years ago

Hello,

Thank you for sharing this project, it really helps me out.

I think there is a small typo in the documentation and examples regarding the full example. When you have used the property 'data', it seems that it has been forgotten to be updated on extra.jsfile, on the following lines:

  1. property: 'session', rather using only property,
  2. ctx.session.counter, rather ctx[property].counter.

Thank you again. Taoufik

EdJoPaTo commented 3 years ago

Looks like you are right. Wanna provide a Pull Request changing that?

TemaSM commented 3 years ago

@Taoufik-B thanks for catch! What about line 13: https://github.com/RealSpeaker/telegraf-session-local/blob/5a6e0a88c077373580638b30f298bced5e4a52ad/examples/extra.js#L13 - it's correct, because we're overriding property field of LocalSession class instance, when calling middleware(): https://github.com/RealSpeaker/telegraf-session-local/blob/5a6e0a88c077373580638b30f298bced5e4a52ad/examples/extra.js#L33 Gonna fix line 38 right now 👌

TemaSM commented 3 years ago

Fixed in https://github.com/RealSpeaker/telegraf-session-local/commit/a921e1506dd0abd81085dd2dc6d68c9c85ec0167

EdJoPaTo commented 3 years ago

@Taoufik-B thanks for catch! What about line 13:

https://github.com/RealSpeaker/telegraf-session-local/blob/5a6e0a88c077373580638b30f298bced5e4a52ad/examples/extra.js#L13

  • it's correct, because we're overriding property field of LocalSession class instance, when calling middleware():

https://github.com/RealSpeaker/telegraf-session-local/blob/5a6e0a88c077373580638b30f298bced5e4a52ad/examples/extra.js#L33

Gonna fix line 38 right now ok_hand

I would avoid to use the line 33 way of doing as the config also specifies that property. It was misleading for @Taoufik-B and for me. Maybe it works but the example should provide a clear way of doing something.

TemaSM commented 3 years ago

Thanks for feedback. Seems to be it's really misleading. Will deprecate argument of .middleware() in next version and remove it from docs and examples completely.