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

expose livehtml autobuild in Makefile + Add API autodoc #971

Merged

Conversation

dvndrsn
Copy link
Contributor

@dvndrsn dvndrsn commented May 22, 2019

Autobuild was previously set up in Makefile, but did not have its requirements frozen in requirements.txt and the command was not documented well. Added API level documentation for parameters and arguments for the core graphene objects.

  • Added make docs-live target to the root project Makefile. This starts a live http server which refreshes with docs changes.
  • Added sphinx-autobuild to documentation requirements.
  • Added autodoc configuration to generate documentation from classes automatically.
  • API documentation attributes and parameters for core graphene objects and interfaces.
  • General tidying and documentation.
  • Fixed a flakey test involving datetime.now() by setting static values for times in tests.

@dvndrsn dvndrsn changed the title expose livehtml autobuild in Makefile expose livehtml autobuild in Makefile + Add API autodoc May 25, 2019
@changeling
Copy link

changeling commented May 25, 2019

I've been building a list of issues and suggestions for the docs over the last little while, and in a few PRs, you've managed to address most of them, at least as a very solid foundation. Kudos! One of the biggest was the lack of an fundamental autodoc style API reference. Much needed! Thank you!

Here's hoping that bit, at the very least, is adopted ASAP.

@dvndrsn
Copy link
Contributor Author

dvndrsn commented May 25, 2019

@changeling thanks! please contribute some suggestions for documentation in this issue: #925

I'm hoping that with a good base document we can soon prioritize things into small chunks that more folks can contribute and polish it up nicely.

@changeling
Copy link

@dvndrsn, will do. I've been away for a week(-ish) in real life, so lemme go through my notes to catch back up. I've begun basic FAQs in the graphene and graphene-django wikis, in hopes folks might contribute recipes and pain-points, and to serve as a harvesting area for documentation development, but have only offered a couple of bits there before life happened. :)

I'd like to be more involved in the docs development going forward, and will look at my notes to see what, if anything, you've not already addressed in your PRs.

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

This is fantastic! I've added some (quite nitpicky) comments but overall this is so great.

@dvndrsn
Copy link
Contributor Author

dvndrsn commented Jun 4, 2019

Thanks for the feedback @jkimbo! I accepted your suggestions and replied to your comment.

@dvndrsn dvndrsn requested review from changeling and phalt as code owners June 4, 2019 21:50
Copy link

@changeling changeling left a comment

Choose a reason for hiding this comment

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

Looks good, though mebbe run black on the docstings code snippets.


.. code:: python

age = graphene.String(

Choose a reason for hiding this comment

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

Might want to run black on code snippets in docstrings here.

Choose a reason for hiding this comment

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

    age = graphene.String(
        # Boolean implicitly mounted as Argument
        dog_years=graphene.Boolean(description="convert to dog years"),
        # Boolean explicitly mounted as Argument
        decades=graphene.Argument(graphene.Boolean, default_value=False),
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I omit graphene.<Whatever> like you did in your PR?

Choose a reason for hiding this comment

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

I'd vote yes on this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes from me too

@dvndrsn dvndrsn force-pushed the expose-docs-commands-in-makefile branch from 947c00c to 67f6976 Compare June 4, 2019 23:35
Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

Some more suggestions (dropping the graphene in places) just for you to easily accept them. Happy to see this merged whenever you want though.


.. code:: python

class NameFormat(graphene.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class NameFormat(graphene.Enum):
class NameFormat(Enum):

Copy link

Choose a reason for hiding this comment

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

I think it's better to keep graphene to show exactly which Enum we mean.

Choose a reason for hiding this comment

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

This may be worth a larger conversation. Naming collisions with python modules like enum.Enum, for example. Perhaps a naming protocol going forward?

As @ProjectCheshire pointed out in https://graphenetools.slack.com/archives/CGR4VCHUL/p1559524808014100, core-next has these types available as, for example:

from graphql import (
    GraphQLSchema, GraphQLObjectType, GraphQLField, GraphQLString)

See https://graphql-core-next.readthedocs.io/en/latest/intro.html#getting-started

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep the import close in all examples to avoid confusion.

Agree with @phalt that the clash for name Enum is a little confusing. I do prefer the shorter names in code as these objects are used very often in the schema. GraphQL as a prefix seems like overkill in most cases (especially given the namespace graphql and graphene for the module).


.. code:: python

class Person(graphene.InputObjectType):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class Person(graphene.InputObjectType):
class Person(InputObjectType):


.. code:: python

class HasAddress(graphene.Interface):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class HasAddress(graphene.Interface):
class HasAddress(Interface):


.. code:: python

class CreatePerson(graphene.Mutation):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class CreatePerson(graphene.Mutation):
class CreatePerson(Mutation):


.. code:: python

class SearchResult(graphene.Union):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class SearchResult(graphene.Union):
class SearchResult(Union):

phalt
phalt previously requested changes Jun 5, 2019
Copy link

@phalt phalt left a comment

Choose a reason for hiding this comment

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

This is an awesome effort well done! Few minor points and please pin the version in requirements.

@@ -1,4 +1,5 @@
# Required library
Sphinx==1.5.3
sphinx-autobuild
Copy link

Choose a reason for hiding this comment

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

please pin this to a specific version


.. code:: python

class NameFormat(graphene.Enum):
Copy link

Choose a reason for hiding this comment

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

I think it's better to keep graphene to show exactly which Enum we mean.

@dvndrsn
Copy link
Contributor Author

dvndrsn commented Jun 5, 2019

I'm going to try to get to these comments by the weekend so this can be merged. Lil crazy this week.

@@ -67,6 +67,28 @@ def from_enum(cls, enum, description=None, deprecation_reason=None): # noqa: N8


class Enum(six.with_metaclass(EnumMeta, UnmountedType, BaseType)):
"""
Enum type defintion

Choose a reason for hiding this comment

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

Suggested change
Enum type defintion
Enum type definition

dog_years=graphene.Boolean(description='convert to dog years'),
# Boolean explicitly mounted as Argument
decades=graphene.Argument(graphene.Boolean, default_value=False)
)

Choose a reason for hiding this comment

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

Suggested change
)
age = String(
# Boolean implicitly mounted as Argument
dog_years=Boolean(description="convert to dog years"),
# Boolean explicitly mounted as Argument
decades=Argument(Boolean, default_value=False),
)

@dvndrsn dvndrsn requested a review from phalt June 9, 2019 13:52
@dvndrsn dvndrsn dismissed phalt’s stale review June 9, 2019 13:54

Addressed comments in docs and pinning dependencies.

@dvndrsn
Copy link
Contributor Author

dvndrsn commented Jun 9, 2019

@jkimbo @ekampf @ProjectCheshire - this is ready to merge now.

@jkimbo jkimbo merged commit da1359e into graphql-python:master Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants