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

fix: use type-specific methods in public clients, Value in internal clients #90

Merged
merged 10 commits into from
Jan 12, 2024

Conversation

anitarua
Copy link
Collaborator

@anitarua anitarua commented Jan 10, 2024

addresses #69

First approach: Allows users to pass String and Binary (List) arguments to functions directly by using the dynamic type, while converting to StringValue or BinaryValue internally.

After discussion, we settled on creating public-facing methods that accept String keys and String/Binary values, but internal clients still use Value as a way to create a type union of the two

@anitarua anitarua marked this pull request as ready for review January 10, 2024 20:00
import 'package:momento/momento.dart';
import 'package:momento/src/errors/errors.dart';

Value getStringOrBinaryFromDynamic(dynamic value, String parameterName) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a really cool thing to try!

I'm guessing the editor isn't able to give any type hints for this at all though? and it would be a runtime error if you just tried to pass in like, some random object type as the argument to one of these dynamic args? If so, lemme noodle on that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If type hints are a good trade for an uglier API, we could add the type to the function name like we do with some of the collection methods in Java: dictionarySetFieldsStringBytes.

@cprice404 cprice404 requested a review from a team January 11, 2024 17:46
@malandis
Copy link

malandis commented Jan 11, 2024

In my reading about Dart, I see that:

  • Dart does not support union types
  • Dart does not support implicit conversions
  • Dart does not support overloading
  • Dart does support generics and generic constraints, but not sealed classes (T extends P but we cannot control who derives from P)
  • Dart does support extension methods

I believe our options are then:

  • Use a wrapper type that the user has to instantiate (previous solution)
  • Use a wrapper type and implement extension methods so the user has to explicitly convert but it's somewhat easier (not explored): "key".asValue()
  • Mangle the names to include the type information as @nand4011 pointed out
  • Use the dynamic keyword and document the allowed types

I see the strong typing is one of Dart's selling points, and I do not have an intuition as to how kosher it is to use dynamic. That said I have looked at popular Dart libraries that could face this problem to see their solution:

Are there any official statements on the matter? These two GitHub issue threads show a lot of folks are unsatisfied about this:

Show no sign of quick resolution and that JSON parsing is an exemplar of the problem and a sore point for devs.

I am leaning toward using extension methods or the mangled function names.

@anitarua anitarua changed the title fix: use dynamic in public clients, Value in internal clients fix: use type-specific methods in public clients, Value in internal clients Jan 11, 2024
Comment on lines 19 to 25
Future<GetResponse> get(String cacheName, String key);

Future<SetResponse> set(String cacheName, Value key, Value value,
Future<SetResponse> set(String cacheName, String key, String value,
{Duration? ttl});

Future<DeleteResponse> delete(String cacheName, Value key);
Future<SetResponse> setBinary(String cacheName, String key, List<int> value,
{Duration? ttl});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opted to allow only String keys, but can provide options for allowing binary keys too if preferable

cprice404
cprice404 previously approved these changes Jan 11, 2024
Copy link

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a good compromise to me.

The only suggestion I might throw out there is to consider doing a find-and-replace on the word Binary with the word Bytes, I think that might be a little more clear and a little more consistent with naming in other SDKs, but I don't feel strongly about it and am fine shipping like this if you or others prefer Binary.

Copy link

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes

@anitarua anitarua merged commit 50894b7 into main Jan 12, 2024
2 checks passed
@anitarua anitarua deleted the string-binary-values branch January 12, 2024 00:28
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

Successfully merging this pull request may close these issues.

4 participants