Create SchemaElement hierarchy #249

pmantica1 · 2019-05-01T14:05:38Z

No description provided.

pmantica1 · 2019-05-01T14:06:41Z

graphql_compiler/schema_generation/graphql_schema.py

-            # represent the edge field as the GraphQL type List(edge_endpoint_type_name).
-            result[field_name] = GraphQLList(graphql_types[edge_endpoint_type_name])
+    if not schema_element.is_non_graph:
+        outbound_edges = (


The change below is essentially just shifting everything 4 spaces to the right and dealing with the 100 character limit.

pmantica1 · 2019-05-01T14:07:55Z

graphql_compiler/schema_generation/schema_graph.py

-    ELEMENT_KIND_EDGE = 'edge'
-    ELEMENT_KIND_NON_GRAPH = 'non-graph'
+    # pylint: disable=metaclass-assignment
+    __metaclass__ = ABCMeta


I assigned the metaclass to ABCMeta in order to make the class an abstract class in a python2/python3 compatible way.

pmantica1 · 2019-05-01T14:17:33Z

graphql_compiler/schema_generation/schema_graph.py

@@ -88,19 +89,15 @@ def get_superclasses_from_class_definition(class_definition):
    return []


+@six.add_metaclass(ABCMeta)


Tested that this works as expected for python2/python3. In other words, tested that we get an error if we try to instantiate a class with this decorator, but we don't if we instantiate a subclass that overrides its abstract methods.

coveralls · 2019-05-01T14:19:59Z

Coverage decreased (-0.1%) to 93.612% when pulling 97ff155 on create-schema-element-hierarchy into f6079c6 on refactor-schema-graph.

obi1kenobi

Overall looks good, just a few minor suggestions and nitpicks.

obi1kenobi · 2019-05-01T14:33:26Z

graphql_compiler/schema_generation/schema_graph.py


-    def __init__(self, class_name, kind, abstract, properties, class_fields):
+    @abstractmethod


__init__ is not abstract, it's implemented. This should get removed.

https://stackoverflow.com/questions/44800659/python-abstract-class-init

obi1kenobi · 2019-05-01T14:35:54Z

graphql_compiler/schema_generation/graphql_schema.py

+        inbound_edges = (
+            ('in_{}'.format(in_edge_name),
+             schema_graph.get_element_by_class_name(in_edge_name).properties[
+                 EDGE_SOURCE_PROPERTY_NAME].qualifier)


Do we still want qualifier to exist as a field? How does this fit in with using the GraphQL type system for types? Assuming this is staying for now and then going away in a subsequent PR?

Yes this is going away in a future PR. In order to remove the qualifier field, I first need to remove links as properties and make the SchemaGraph use GraphQL's type system.

obi1kenobi · 2019-05-01T14:38:23Z

graphql_compiler/schema_generation/schema_graph.py

+        return isinstance(self, NonGraphElement)
+
+    @abstractmethod
+    def __str__(self):


Not sure why the linter didn't complain here, but these functions need docstrings. Also, look at the implementation of CompilerEntity for an example of how you can implement __str__ and __repr__ generically. You'll have to do a bit of magic with the constructor, and to get the names of args defined by subtypes' constructors you'll have to pass them in as kwargs to this constructor, but it can work and simplify your code overall.

I think that linter was in arc lint, not sure if we picked it up.

obi1kenobi · 2019-05-01T14:39:43Z

graphql_compiler/schema_generation/schema_graph.py


    def freeze(self):
        """Make the SchemaElement's connections immutable."""
        self.in_connections = frozenset(self.in_connections)
        self.out_connections = frozenset(self.out_connections)

+
+class Vertex(GraphElement):


I'm a bit worried that Vertex and Edge might get confused with the actual data vertices and edges. Maybe VertexType/EdgeType or something like that?

obi1kenobi · 2019-05-01T14:40:30Z

graphql_compiler/schema_generation/schema_graph.py

@@ -88,19 +89,15 @@ def get_superclasses_from_class_definition(class_definition):
    return []


+@six.add_metaclass(ABCMeta)
 class SchemaElement(object):


We probably want @six.python_2_unicode_compatible here as well, check out the docs for it.

obi1kenobi · 2019-05-01T14:41:54Z

graphql_compiler/schema_generation/schema_graph.py

+        #
+        # For non-graph classes, these properties are always empty sets.
+        self.in_connections = set()
+        self.out_connections = set()

    def freeze(self):


Let's define freeze() on SchemaElement as well, and make sure to always call the parent class' freeze() function.

obi1kenobi · 2019-05-01T14:42:50Z

graphql_compiler/schema_generation/schema_graph.py

+        super(NonGraphElement, self).__init__(class_name, abstract, properties, class_fields)
+
+        # Non-edges must not have properties like "in" or "out" defined, and
+        # must not have properties of type "Link".


Presuming the "type Link" stuff is going away in the next PR?

obi1kenobi · 2019-05-01T14:44:42Z

graphql_compiler/schema_generation/schema_graph.py

                    raise AssertionError(u'For property "{}" of non-abstract edge class "{}", '
                                         u'no such subclass-of-all-elements exists.'
                                         .format(property_name, class_name))

-            self._elements[class_name] = SchemaElement(class_name, kind, abstract,
-                                                       property_name_to_descriptor, class_fields)
+            self._elements[class_name] = kind(class_name, abstract, property_name_to_descriptor,


If you are going to call it as a constructor, call it element_cls or something like that, to make it obvious that it's a type and it can be called as a constructor.

obi1kenobi · 2019-05-01T14:45:21Z

graphql_compiler/schema_generation/schema_graph.py

@@ -537,13 +580,13 @@ def _set_up_schema_elements_of_kind(self, class_name_to_definition, kind, class_
                        property_name_to_descriptor[property_name] = property_descriptor

                if (property_name not in property_name_to_descriptor and not abstract and
-                        kind == SchemaElement.ELEMENT_KIND_EDGE):
+                        kind == Edge):


It's almost never a good idea to compare types for equality. Use issubclass instead: https://docs.python.org/3/library/functions.html#issubclass

pmantica1 · 2019-05-01T15:00:03Z

In this PR I intend to make the minimal changes necessary to separate each SchemaElement kind into its own class and to build the SchemaElement hierarchy. In future PRs, I intend to remove links as properties, make the SchemaGraph use GraphQL's type system, and remove the PropertyDescriptor's qualifier field.

pmantica1 · 2019-05-01T16:47:30Z

graphql_compiler/schema_generation/schema_graph.py

@@ -460,7 +496,7 @@ def _split_classes_by_kind(self, class_name_to_definition):
        self._edge_class_names = frozenset(self._edge_class_names)
        self._non_graph_class_names = frozenset(self._non_graph_class_names)

-    def _set_up_schema_elements_of_kind(self, class_name_to_definition, kind, class_names):
+    def _set_up_schema_elements_of_kind(self, class_name_to_definition, kind_cls, class_names):


I am not satisfied with kind_cls, but I will heavily refactor _set_up_schema_elements_of_kind entirely in a future commit anyways. It is too large of a function and a bit hard to understand. I might also split it up into _set_up_vertex_elements, _set_up_edge_elements and _set_up_graph_elements.

graphql_compiler/schema_generation/schema_graph.py

* Create SchemaElement hierarchy (#249) * Refactored SchemaElement to use a class hierarchy * Correctly set metaclass in a python2/python3 compatible way * Nit * Removed abstractmethod decorator from init method * Changed Vertex to VertexType and Edge to EdgeType * Added unicode compatibility decorator * Fixed class name typos * Use issubclass instead of class equality * Added generic string representation * Changed kind to kind_cls * Fixed args bug * Removed __str__ overrides * Added freeze method for all SchemaElements * Made SchemaElement and GraphElement classes truly abstract * Fixed linter errors * Added comment explaining why we make the init method abstract * Decompose _setup_schema_elements_of_kind (#250) * Added _try_get_exact_link_from_superclasses * Created _is_abstract method * Seperated get class fields into its own function * Created _get_element_properties method * Removed allowed_duplicate_properties variable * Nits * Nits * Decomposed _get_element_properties into two functions (#251) * Decomposed _get_element_properties into two functions * Use funcy split instead of partition function * Specified funcy version * Nits (#252) * Fixed stack overflow links * Fixed freeze documentation * Add different setup function for each kind (#254) * Removed _get_element_properties method * Createdseperate setup function for each element kind * Moved no inks defined on non-edge class assertion one scope up * Call _get_link_properties only for _set_up_edge_elements * Modified GraphElements class to deal with *args and **kwargs * Rename root_connections to base_connections * Rename root_connections to base_connections * Renamed _get_link_properties to _get_base_connections * Changed base_connections to be a string instead of a PropertyDescriptor * Documented the edge type class * Fixed the assertion error documentation * Nit * Added validation for link defintions * Changed spec of functions that created SchemaElement properties * Removed unneeded validation * Removed unneeded link validation * Linter fix * Changed EDGE_SOURCE_PROPERTY_NAME and EDGE_DESTINATION_PROPERTY_NAME to be class attributes of EdgeType * Added key validation for base_connections * Nit * Removed incorrect documentation * Nit * More nits * Moved edge end names to schema_properties * Changed base_connections to base_in_connection and base_out_connection * Switched the SchemaGraph to use GraphQL's type system Sorted imports Fixed linter errors * Nits * Changed graphql_type variable name to maybe_graphql_type * Added removed comment * Added assertions * Fix typo * Filter collections of classes * Added comment explaining why non-graph objects have no fields and interfaces * Nit * Fixed typos * Removed unused import * Fixed tests * Fixed assertion error documentation * Refactored _try_get_base_connection * Fix documentation of _try_get_base_connections * Moved validation into _try_get_graphql_type * Changed maybe_graphql_type to graphql_type * Added GraphQLAny * Refactored get_graphql_type * Fixed linter errors * Nit * Nits * Nit * Add SchemaGraph test for OrientDB edge inheritance * Fixed linter errors * Nit * Add warnings for filtered class collection properties * Nit * Refactor SchemaGraph class collections * Fixed linter issues * Refactored the SchemaGraph constructor into a more general one * Made split_elements_by_kind a getter function instead of a method * Create get_subclass_sets_from_inheritance_sets function * Changed the method for getting inheritance_sets to be a function instead * Splitted _try_get_base_directions into two methods * Made _try_get_base_connections a function * Made _get_end_direction_to_superclasses a function instead of a method * Made link validation a function instead of a method * Made _get_graphql_type a function instead of a method * Made _get_element_properties a function instead of a method * Made _link_vertex_and_edge_types a function * Made the methods for setting up each concrete SchemaElement class a method * Removed the SchemaBuilder class * Fix function names * Removed unnecessary argument * Reduced number of function arguments * Fixed linter errors * Ignore unsupported properties in SchemaGraph. (#265) * Ignore unsupported properties * Nits * Fixed typo * Deal with non-null GraphQL types * Remove GraphQLAny scalar type (#269) * Update documentation * Make imports relative instead of absolute * Moved helper function to the bottom * Deal with non-null GraphQL types * Made the assertion error more intuitive * Nit * Add missing copyright message * Added another missing copyright line * Fixed typo * Moved PropertyDescriptor namedtuple to the SchemaGraph file * Reordered functions and classes in SchemaGraph file * Moved class collection validation to the SchemaGraph * Revert "Moved class collection validation to the SchemaGraph" This reverts commit 2cb4f15. * Create non-graphql objects from the start * Nits * Moved the illegal name prefixes to the SchemaGraph file * Removed Kind enum and replaced _get_class_names_of_kind * Changed module name * Shortened module name * Make get_properties_with_defaults public (#288) * Fix _get_elements_of_class * Allow connections for NonGraphElements (#287) * Allow connections for NonGraphElements * Renamed misleading function * Remove dot * Added missing in_connections and out_connections to the printed fields * Fix linter errors * Allow GraphQL schema edges for NonGraphElements (#291)

Refactored SchemaElement to use a class hierarchy

fa33dd7

pmantica1 commented May 1, 2019

View reviewed changes

Correctly set metaclass in a python2/python3 compatible way

b0223d5

pmantica1 commented May 1, 2019

View reviewed changes

Nit

fe71812

pmantica1 changed the title ~~Refactored SchemaElement to use a class hierarchy~~ Create SchemaElement hierarchy May 1, 2019

obi1kenobi reviewed May 1, 2019

View reviewed changes

Pedro Mantica added 11 commits May 1, 2019 11:03

Removed abstractmethod decorator from init method

da777da

Changed Vertex to VertexType and Edge to EdgeType

a718efb

Added unicode compatibility decorator

5691fd2

Fixed class name typos

3f7137c

Use issubclass instead of class equality

d807472

Added generic string representation

e95aac4

Changed kind to kind_cls

f1ed828

Fixed args bug

c65dc95

Removed __str__ overrides

6f83a72

Added freeze method for all SchemaElements

b3cc68c

Made SchemaElement and GraphElement classes truly abstract

f44d1c9

pmantica1 commented May 1, 2019

View reviewed changes

Pedro Mantica added 2 commits May 1, 2019 13:02

Fixed linter errors

fc92bbf

Added comment explaining why we make the init method abstract

97ff155

bojanserafimov merged commit 01e2716 into refactor-schema-graph May 1, 2019

bojanserafimov deleted the create-schema-element-hierarchy branch May 1, 2019 19:21

obi1kenobi reviewed May 2, 2019

View reviewed changes

graphql_compiler/schema_generation/schema_graph.py Show resolved Hide resolved

obi1kenobi reviewed May 2, 2019

View reviewed changes

graphql_compiler/schema_generation/schema_graph.py Show resolved Hide resolved

obi1kenobi reviewed May 2, 2019

View reviewed changes

graphql_compiler/schema_generation/schema_graph.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create SchemaElement hierarchy #249

Create SchemaElement hierarchy #249

pmantica1 commented May 1, 2019

pmantica1 May 1, 2019

pmantica1 May 1, 2019

pmantica1 May 1, 2019

coveralls commented May 1, 2019 •

edited

obi1kenobi left a comment

obi1kenobi May 1, 2019

pmantica1 May 1, 2019

obi1kenobi May 1, 2019

pmantica1 May 1, 2019

obi1kenobi May 1, 2019

bojanserafimov May 1, 2019

obi1kenobi May 1, 2019

obi1kenobi May 1, 2019

obi1kenobi May 1, 2019

obi1kenobi May 1, 2019

obi1kenobi May 1, 2019

obi1kenobi May 1, 2019

pmantica1 commented May 1, 2019

pmantica1 May 1, 2019

		@@ -88,19 +89,15 @@ def get_superclasses_from_class_definition(class_definition):
		return []


		@six.add_metaclass(ABCMeta)


		def __init__(self, class_name, kind, abstract, properties, class_fields):
		@abstractmethod

Create SchemaElement hierarchy #249

Create SchemaElement hierarchy #249

Conversation

pmantica1 commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2019 • edited

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmantica1 commented May 1, 2019

Choose a reason for hiding this comment

coveralls commented May 1, 2019 •

edited