Giter Club home page Giter Club logo

Comments (7)

RoyWolfe avatar RoyWolfe commented on July 2, 2024 1

Makes sense to focus on your existing roadmap first and leave the uint/int thing for a future update!

Re: Static helpers, I don't think my experience alone here should be the deciding factor in this. Everything else being equal, I think more documentation of what would be the intended/expected way to use the library could be beneficial. The documentation site lists only two fairly straightforward examples (as far as I can tell). It could help to see a few more advanced use cases, so it's easier discoverable that all these wonderful types exist and what the idiomatic way is to use them?

Maybe I'm coming from a point of view where I knew nothing about the Telegram API to begin with, so I wasn't aware what options and choices I should be on the look-out for. The static helper methods would for sure signpost this, I can't really argue, but onboarding onto your library's "way of working" may sidestep the problem altogether?

--

Since you're also offering, I'd like to raise one other question, semi-related to this: Why is the CaptionEntities property on the SendPhotoUrl type IEnumerable? The context here is having thought about dynamic composition -- some of my photos have a description, some don't. If they don't it looks odd if the title is in bold. First instinct was to go sendPhotoUrl.CaptionEntities.Add(...), but can't add to an IEnumerable. Ofc I can just construct a List outside of the new SendPhotoUrl (){...}, so I am mostly curious about the design thought!

And in turn thank you for making this library; I agree with your reasoning about why you made this over using That Popular One, and I'm enjoying using it! Making those Telegram messages just works! ♥

from telegram.bots.

RoyWolfe avatar RoyWolfe commented on July 2, 2024 1

Makes perfect sense! In a way, I wish this immutable record approach was more common in the wider world of .net (at least from my experience with "boring LOB applications"). And I certainly agree that that RNG example should be a rare usecase to be encountered in the wild -- it seems like someone is actively trying to shoot themselves in the foot that way, given there are easy workarounds like the one you outlined.

I'll also admit I did not think of generator functions, yield, and all the other shiny tricks you can do with GetEnumerator. Good thinking!

Thank you once more for the detailed explanation! Especially seeing that it was mostly indulging my curiosity, rather than actually experiencing any problem, I appreciate you sharing all the information with me.

from telegram.bots.

AmanAgnihotri avatar AmanAgnihotri commented on July 2, 2024

Hi! Thank you for using this library for your use-cases. I believe the situation you have can be resolved by taking a look at the MessageEntity class. I'll post it here too:

namespace Telegram.Bots.Types;

using System;

public sealed record MessageEntity(
  MessageEntityType Type,
  uint Offset,
  uint Length)
{
  public Uri? Url { get; init; }

  public User? User { get; init; }

  public string? Language { get; init; }

  public string? CustomEmojiId { get; init; }
}

As you can see from it, the fields which are optional for a MessageEntity are available (including the Url which you are interested in).

I believe that you will get what you are wanting if you were to write something as:

var caption = $"{photoToPost.Title}\n\nView original on Flickr";

var request = new SendPhotoUrl(_options.TargetChatId, photoToPost.PhotoUrl, UriKind.Absolute))
{
    Caption = caption,
    CaptionEntities = new List<MessageEntity>
    {
        new MessageEntity(MessageEntityType.Bold, 0, (uint)photoToPost.Title.Length),
        new MessageEntity(MessageEntityType.TextLink, (uint)photoToPost.Title.Length, 23)
        {
          Url = photoToPost.WebUrl
        }
    }
};

return await _botClient.HandleAsync(request);

I also notice that you had to do a casting of your integers to uint. I used uint to emphasize the non-negative nature of those values but it may be making the code uglier than it should be. Telegram server should anyway report an error if these values were sent as negative so maybe changing the types of Offset and Length from uint to int in the MessageEntity class would be better. Although, it will also be backward incompatible change. Any feedback on this part from the user experience point of view?

Anyhow, I hope the suggestion resolves your issue.

from telegram.bots.

RoyWolfe avatar RoyWolfe commented on July 2, 2024

Good evening!

And thank you kindly for the thorough and quick response. I appreciate you taking the time to go into the depth of a reply even on something that turns out to be an oversight on my part! (One that I feel embarrassed about, given that I am using object initialisers right there next to it. I don't know why I didn't check the properties on the MessageEntity type. Mea culpa!)

When using the code as you suggested everything works fine! Thank you again for the help.


Regarding uint, I can definitely see where you're coming from and it is more expressive, in the sense that it signposts "nothing negative here". I wouldn't hold that against you as design choice. However, in my experience and in general, APIs prefer using int over uint unless there is a significant reason to enforce this. In the dotnet base library, this is informed by the common language spec not making room for uints; so any software that needs to work with non-c# / non-f# / ... languages on the .net runtime cannot use uint at all. So the base libraries are designed not to use them unless necessary.

(Some sauce: https://learn.microsoft.com/en-us/dotnet/standard/language-independence -- NB just in case: int/uint would be Int32 / Uint32 by default; Uint16 seemingly exists in the CLS, so ushort would work ... but what API uses shorts nowadays?)

This might not really be necessary when talking about a Telegram bot, but it does mean that whenever we're having interactions between Telegram.Bots and the base class library, casting will be necessary. (See e.g. the point in case in my example; I can't off the top of my mind think of readily available functions to dynamically point at parts of the caption that use uint instead of int.) To me it would feel more idiomatic to stick with int even though it most certainly would allow for values that are clearly nonsensical as they are negative.

If you want to change to use int here, now is probably the best time -- if your framework gains more and more popularity it'll be harder to introduce the breaking change. On the other hand, I don't find it a big issue to risk breaking implementations over, and this might be one to take onboard for the next major version of the framework. After all, my work is not mission critical; and I don't think I'll ever have negative offsets, so I feel safe with these casts. (Otherwise, there's always Convert.ToUint32()...)

from telegram.bots.

AmanAgnihotri avatar AmanAgnihotri commented on July 2, 2024

Great that your issue is resolved! We have all been there, missing that key which we deem simple in hindsight. Haha.

I agree with your views on the uint and int usage here.

I will make this library compliant with Bot API 6.3 first as it's halfway there and then make a major version update by replacing all uint to int for more cleaner usage. Already lagging in updates as Bot API 6.6 has also come, although the changes are not that many to reach compliance till that level.

Seeing as you had this issue with the MessageEntity, I think I will also add some static helper methods which can be invoked like MessageEntity.CreateTextLink(...) to make it easier to not miss out on these optional properties. I did not add them earlier because I was anyway not fully sure on what all can be used in the different possibilities which a MessageEntity can have but I will give it a go.

My goal with this library had always been complete type safety and clean UX, so these ideas seem in alignment.

Thanks for using this library again and can feel free to ask any other questions as they come to you.

from telegram.bots.

AmanAgnihotri avatar AmanAgnihotri commented on July 2, 2024

I did not put much work on the documentation site per se and instead opted to have the README file in this repository to cover the typical scenarios. Writing an exhaustive documentation sure is a time-taking process and I have not, as of yet, invested that kind of time to it but will consider. I should probably remove the link to the documentation site as it's more of a rationale site now. Haha.

Regarding your question on why I have used IEnumerable<MessageEntity>? for CaptionEntities, it's so that the user can make use of any implementation which is enumerable, including generator functions (ones which use yield). You can use a List<MessageEntity> but can likewise just use other constructs like lists, arrays, stacks, queues, sets, etc., like:

using System.Collections.Immutable;

public sealed class Test
{
  private readonly IEnumerable<int> _values;

  public Test()
  {
    _values = new List<int>();
    _values = new int[2];
    _values = ImmutableList<int>.Empty;
    _values = ImmutableArray.Create(1, 2);
    _values = ImmutableList.Create(1, 2);
    _values = ImmutableQueue.Create(1, 2);
    _values = ImmutableStack.Create(1, 2);
    _values = new []{1, 2};
    _values = GetValues();
    // etc..
  }

  public static IEnumerable<int> GetValues()
  {
    yield return 1;
    yield return 2;
  }
}

The requests and types being a record with no explicit way to update properties once the record has been initialized ensures that there's less ambiguity regarding what the values are when they are sent to the Telegram's server. If I allowed the requests and other types to be mutable, it could allow for code which would end up being difficult to understand. Immutability allows for easier reasoning here and hence no Add on CaptionEntities once you have already initialized the SendPhotoUrl object.

I could have also used IReadOnlyList<T> for the requests but then they would force you to have your list materialized and would not work for enumerables which are not lists or arrays like generator functions, stacks, queues, etc.

You will see that for all the types that get returned from Telegram, I do use IReadOnlyList<T> instead of IEnumerable<T>. This ensures that the compiler can never complain about "possible multiple enumerations".

One "issue", so to say, with my using IEnumerable<T> in requests could be if you were to both store the request's serialized state and send it to Telegram while using some randomized approach to preparing the IEnumerable<T> on the fly using generator functions. In such a case, the list would get materialized twice, once during serialization to storing it elsewhere and another when it's getting sent to the Telegram server. But that is very likely never going to happen in the use-cases for which this bot API is being used so it'll not be an issue.

For example, if I had the following:

static IEnumerable<int> GetNewValues()
{
  yield return RandomNumberGenerator.GetInt32(10);
  yield return RandomNumberGenerator.GetInt32(10);
  yield return RandomNumberGenerator.GetInt32(10);
  yield return RandomNumberGenerator.GetInt32(10);
}

IEnumerable<int> values = GetNewValues();

Console.WriteLine(string.Join(", ", values));
Console.WriteLine(string.Join(", ", values));

The above will output two different lists, one of each print. The compiler will also warn for "possible multiple enumerations", so should likely fix it anyway and not use such code.

But doing the following will give the expected:

IEnumerable<int> GetFixedValues()
{
  return new List<int>
  {
    RandomNumberGenerator.GetInt32(10),
    RandomNumberGenerator.GetInt32(10),
    RandomNumberGenerator.GetInt32(10),
    RandomNumberGenerator.GetInt32(10),
  };
}

IEnumerable<int> fixedValues = GetFixedValues();

Console.WriteLine(string.Join(", ", fixedValues));
Console.WriteLine(string.Join(", ", fixedValues));

The above will output the same list twice. This is how users will generally make use of such library, so it seemed fine to me to have IEnumerable<T> in the request objects so that the user could also use other enumerables which are not lists/arrays, including generator functions if need be.

from telegram.bots.

AmanAgnihotri avatar AmanAgnihotri commented on July 2, 2024

Always happy to help. :)

Good luck with your app development!

from telegram.bots.

Related Issues (6)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.