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

Missing 'GetValueAsString()' method #111

Open
DennisVM-D2i opened this issue Oct 23, 2023 · 8 comments
Open

Missing 'GetValueAsString()' method #111

DennisVM-D2i opened this issue Oct 23, 2023 · 8 comments

Comments

@DennisVM-D2i
Copy link

DennisVM-D2i commented Oct 23, 2023

Rather than having to check to cast to every specific Haystack type/'Value' - when you're traversing an unknown database, it would help so-so much for all of the 'Value' classes to inherit an overridable/virtual 'GetValueAsString()' method to convert/To-String the internal ('.Value') value to a string; possibly with the optional argument to allow you to influence the type of conversion - for when you do happen to know the underlying type - e.g. select either a short or long string conversion for DateTimes.

(Or maybe at the very-very least - although less desirable, an extension method for each class with the same method name.)

@Breederveld
Copy link
Contributor

Hi @DennisVM-D2i, the reason this has not been added is because not all values have a clear-cut way to stringify them. I suggest you use the most applicable writer for the job and use it to get the string values.
For example, the ZincWriter.ToZinc static method can be used to convert any haystack value to a Zinc string representation.

@DennisVM-D2i
Copy link
Author

DennisVM-D2i commented Jan 18, 2024

Hi Chris, thanks for you're reply.

The thought was that if you mark the methods as 'virtual', then a client could decide to consistently override them, so you'd be giving the benefit of an out-of-the-box starting point.

I personally feel that having to keep having repeated switch statements to decide how to convert from every type - or even one method to do it, just seems like it could (if not should) sit within/be provided by the client.

I was even extending my thinking to have 2 other methods; one being the 'GetValueAsHsTypeString()', that provides a more descriptive & structured rendition; e.g. here are a (related) pair sample of the two methods, both for the 'HaystackBinary':

// DVM: Added: public override string GetValueAsHsTypeString() { return $"<# HsBinary: Mime=\"{Mime}\" #>"; }

// DVM: Added: public override string GetValueAsString() { return Mime; }

For the 'HaystackNumber':

// DVM: Added: public override string GetValueAsHsTypeString() { return $"<# HsNumber: Value=\"{Value}\", Unit=\"{Unit}\" #>"; ; }

// DVM: Added: public override string GetValueAsString() { return $"'{Value}' - '{Unit}'"; }

I'm still trying to form my own opinion of the format of the 'HsType' string, so maybe like this following alternative instead - just to shorten the left hand-side of it (for ease of reading/viewport-landscape before scrolling to the right is required), by moving the proper/long-form type name to the right (- which maybe one day could just-in-case allow for a 3rd-party-namespaced type, e.g. 'AbcCorp:OctetArray'):

public override string GetValueAsHsTypeString() { return $"<# HsBin: Mime=\"{Mime}\" Type="HsBinary" #>"; }

The other method would be to return the value as an 'object' (rather than convert it to a 'string') - 'GetValueAsObject()'.

@Breederveld
Copy link
Contributor

Breederveld commented Jan 18, 2024

Hi Dennis,

Perhaps I'm misunderstanding you, but if I would add virtual methods to the types you'd have to convert the types coming from the library to your types before being able to actually use them. Also you'd have to implement every type individually.

I would just suggest you add a simple extension method on HaystackValue that uses ZincWriter.ToZinc (or similar logic) and use that on your values instead if you prefer centralizing how the ToString implemention should work.

public static HaystackValueExtensions
{
  public static string ToYourString(this HaystackValue value) => ZincWriter.ToZinc(value);
}

@DennisVM-D2i
Copy link
Author

Hi I get what you are saying, but if you were to go that one implementation/method route, I would still question whether the method should be an overrideable method in a base class; so if you really didn't like this one-size-fits all version, you could customise or wholeheartedly replace it (for a specific type or two).

One benefit of the many-implementations, is the opportunity to render it better/closer to the needs of the specific type.

But as a generic starting point, something like what you've got makes sense.

But maybe my thinking was more a non-Zinc/more human-friendly rendition.

At the very least, a base class feels like it could prove useful, even if you decide not in this specific case.

@Breederveld
Copy link
Contributor

Breederveld commented Jan 18, 2024

I'm still trying to follow your reasoning here; having some virtual method with the sole purpose over it being overridable, you'd have to do the following on the client to make it work:

  • Override all Haystack Types
  • Create mapping logic form the base types to your custom types
  • Create a custom HaystackClient that handles the custom mapping logic

All this just to have a ToString method, that you can also add using a single Extension method.

Perhaps I'm just missing your point and you can provide a simple example PR to show what direction you are thinking of?

@Breederveld
Copy link
Contributor

Hi @DennisVM-D2i , do you want to follow up on this issue, or is it fair to close it at this time?

@DennisVM-D2i
Copy link
Author

DennisVM-D2i commented Mar 4, 2024 via email

@Breederveld
Copy link
Contributor

Sure, I'll wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants