SPIE-Worksphere / haystack-csharp

Haystack .net client
Academic Free License v3.0
15 stars 6 forks source link

Missing 'GetValueAsString()' method #111

Open DennisVM-D2i opened 8 months ago

DennisVM-D2i commented 8 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

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 commented 4 months ago

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

DennisVM-D2i commented 4 months ago

(Hi, I'm trying to get the time to get back to this; sorry.)

Dennis McEnaney Technical Lead tel 020 7481 1655<tel:+442074811655>

[Smart Spaces] [Linkedin]https://www.linkedin.com/company/smartspacesapp[Facebook]https://www.facebook.com/smartspacesapp[Twitter]https://twitter.com/SmartSpacesApp[Instagram]https://www.instagram.com/smartspaces_/

www.smartspaces.apphttp://www.smartspaces.app Registered Address: D2 Interactive, Portsoken House, 155-157 Minories, London, EC3N 1LJhttps://goo.gl/maps/2yY7N3YNd7E2

This email may be private and confidential, and contain legally privileged information. If you are not the addressee you should not disclose, copy, circulate or in any other way use the information contained in this transmission. Such unauthorised use is prohibited and may be unlawful. If you are not the intended recipient, please contact us immediately. [https://www.smartspaces.app/signature/lhspacer.gif][EG Tech Awards] [gcuk Coworking Awards] [Property Awards Finalist 2022] https://awards.propertyweek.com/propertyawards2020/en/page/2020-shortlist [Construction Excellence SECBE Awards] [BCO Awards] [Cyber Essentials Plus] https://www.smartspaces.app/signature/CyberEssentialsScheme_Certificate.pdf [SmartScore AP]

[Building Engagement]

From: Chris Breederveld @.> Sent: Saturday, March 2, 2024 7:07 PM To: SPIE-Worksphere/haystack-csharp @.> Cc: Dennis V McEnaney @.>; Mention @.> Subject: Re: [SPIE-Worksphere/haystack-csharp] Missing 'GetValueAsString()' method (Issue #111)

Hi @DennisVM-D2ihttps://github.com/DennisVM-D2i , do you want to follow up on this issue, or is it fair to close it at this time?

- Reply to this email directly, view it on GitHubhttps://github.com/SPIE-Worksphere/haystack-csharp/issues/111#issuecomment-1974880997, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIUOSLXO5SLP3XVN5HNO6W3YWIPOHAVCNFSM6AAAAAA6MQDVCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZUHA4DAOJZG4. You are receiving this because you were mentioned.Message ID: @.**@.>>

Disclaimer

The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorized to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been automatically archived by Mimecast, a leader in email security and cyber resilience. Mimecast integrates email defenses with brand protection, security awareness training, web security, compliance and other essential capabilities. Mimecast helps protect large and small organizations from malicious activity, human error and technology failure; and to lead the movement toward building a more resilient world. To find out more, visit our website.

Breederveld commented 4 months ago

Sure, I'll wait