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

Propositions for class-based API improvements and simplification #143

Open
skovbasa opened this issue Oct 23, 2023 · 1 comment
Open

Propositions for class-based API improvements and simplification #143

skovbasa opened this issue Oct 23, 2023 · 1 comment

Comments

@skovbasa
Copy link
Collaborator

skovbasa commented Oct 23, 2023

  1. Unify field and field_link. Field and Link are needlessly different, while often being used in the same context.
  • Link often requires customly created private Field for low-level graph access optimizations. This spams graph definition with lots of Fields that are basically parts of corresponding Links and are otherwise useless for API clients. Possible solution: for link replace requires with the second resolver, common for all fields (similar to current field funcs).
  • Fields usually share one common resolver, which gives ability to make super efficient queries and is generally one of the things that makes hiku a great tool. BUT it also gives a few problems: never ending if field_name == 'someName': ... statements, which complicate application logic; and the inablity to attach any out-of-the-box typecheckers to field resolvers. Possible solution: for field add a new field-specific resolver (similar to current link funcs).
  • With proposed solutions there is not quite clear difference between two resolvers (common for all fields and field-specific), which may introduce problems in the future. This should be discussed.
  1. Add dataclass-typed Options and change field resolvers to accept them. Same option classes can be used in both resolver annotations and graph declaration.
  2. Require explicit Node key declaration (value returned from link funcs and recieved by field resolvers). This will make typechecking of such types easier and improve understanding of hiku type resolvers.
  3. Return lightweight objects from link functions instead of scalars. Something like Article.link(1) will be much easier to typecheck, will make resolvers for union and interface types much more intuitive, and will remove need for custom Nothing constant in favour of None.
  4. Typecheck field resolvers for return and argument types.
  5. Send context into every resolver. This will greatly simplify internal code, but may impact performance.
  6. Find alternative for @define functions: S.this constructs are not easy to understand and decorators use strings which are prone to errors.
@kindermax
Copy link
Collaborator

kindermax commented Oct 23, 2023

Fields usually share one common resolver, which gives ability to make super efficient queries and is generally one of the things that makes hiku a great tool. BUT it also gives a few problems: never ending if field_name == 'someName': ... statements, which complicate application logic; and the inablity to attach any out-of-the-box typecheckers to field resolvers. Possible solution: for field add a new field-specific resolver (similar to current link funcs).

One thing that comes into mind, is to use specific resolvers for fields, but mark that all of them bound somehow, for example:

Instead of:

def map_user_fields(fields, users):
  def get_field(f, user):
    if f.name == 'id':
      return user.id
    if f.name == 'name':
      return user.name
    if f.name == 'email':
      return user.email
    raise Exception('unhandled field')

   return [ [ get_field(f, user) for f in fields ] for user in users ]

Node('User', [
  Field('id', map_user_fields),
  Field('name', map_user_fields),
  Field('email', map_user_fields),
])

we can do this:

@hiku.field(node="User")
def user_id_field(field, users):
  return [u.id for u in users]

@hiku.field(node="User")
def user_name_field(field, users):
  return [u.name for u in users]

@hiku.field(node="User")
def user_email_field(field, users):
  return [u.email for u in users]


Node('User', [
  Field('id', user_id_field),
  Field('name', user_name_field),
  Field('email', user_email_field),
])

Pros:

  • typing support is easier
  • simpler resolvers
  • in future class-based definition it will be much easier to generate (or call implicitly ) the same code
    @hiku.node
    class User:
      id: int
      name: str
      email: str
     
     # nothing prevents us from implicitly iterate over each user and call `getattr(user, field)` which is the same as defining a 
     # function for each field which will return just the same attribute explicitly

Cons:

  • in plain-old hiku field mapper, if you want to apply custom logic to field, you just do it in an if statement, but in my prototype each custom field handler is automatically a separate function
  • more function calls (though it is not clear if it actually will harm perf since we actually calling get_field private function for each field which is mostly the same )
  • more iterations over users collection
    • possible solution - accept user instead of users and do iteration in hiku internally
    for user in users:
      for field in fields:
         result.append(user_id_field(field, user)) # pseudocode

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