Skip to content

Renames #68

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

Merged
merged 8 commits into from
Nov 18, 2015
Merged

Renames #68

merged 8 commits into from
Nov 18, 2015

Conversation

FugiTech
Copy link
Contributor

@FugiTech FugiTech commented Nov 8, 2015

As discussed in #20 and #44, this renames a bunch of structs and methods. I simply used gorename to do this, there are no actual code changes.

Graphql -> Do
FieldConfigMap -> Fields
FieldConfig -> Field
GQLFRParams -> ResolveParams
GQLFormattedErrorSlice -> FormattedErrors
Name -> PrivateName
Description -> PrivateDescription
GetName -> Name
GetDescription -> Description
GetError -> Error
GetValues -> Values
GetFields -> Fields
GetObjectType -> ObjectType
GetPossibleTypes -> PossibleTypes
GetInterfaces -> Interfaces
GetDirectives -> Directives
GetMutationType -> MutationType
GetQueryType -> QueryType
GetType -> Type
GetTypeMap -> TypeMap

I couldn't use name and description due to encoding/json only working on exported fields.

@sogko
Copy link
Member

sogko commented Nov 10, 2015

Hi @Fugiman

This is great! Thanks for taking this up!
I love the new changes and appreciate that you took the time to list them out 👍🏻

Re: PrivateName and PrivateDescription, like @Fugiman has described, I think we have no other choice, but to have "exported but private fields".
(Exported for json/encode, but private because we want to restrict/discourage write access to these fields and enforce/encourage read access through Name(), for e.g)

Besides prefixing it with Private* to denote these exported-but-privates, anyone has any other suggestions to consider before we commit to this?

Can I suggest writing a little comment for at least one of the Private* field so that we could document the design decision?

Cheers!

@sogko sogko mentioned this pull request Nov 10, 2015
@chris-ramon
Copy link
Member

Great!, thanks for working on this @Fugiman! 🌟

What about implementing json.Marshaler, so we can go for private lowercase field names and avoid Private* suffix.

I went ahead and made a simple example.

@sogko
Copy link
Member

sogko commented Nov 12, 2015

@chris-ramon That's a great idea 👍🏻

If you want to try this approach, IIRC you might want to take a look at defaultResolveFn() in executor.go and change the implementation to use json.Marshal() to marshal p.Source to JSON and extract the value. (Currently it uses reflect to manually extract the value for the given field name).

@chris-ramon chris-ramon mentioned this pull request Nov 17, 2015
8 tasks
@chris-ramon
Copy link
Member

I think this is ready to go, I've reviewed and looks good to to me, if someone would like to share ur thoughts on this please do 👍

Great!, thanks for working on this @Fugiman!

What about implementing json.Marshaler, so we can go for private lowercase field names and avoid Private* suffix.

I went ahead and made a simple example.

json.Marshaler implementation being handle on #75, we want to merge this one first then #75.

Again thanks for ur work on this @Fugiman 🌟 I've discovered various great go tools since you've being contributing!, thanks for that as well 😄

@sogko
Copy link
Member

sogko commented Nov 18, 2015

This looks good to me as well 👍🏻

sogko added a commit that referenced this pull request Nov 18, 2015
Renamed exported structs and methods for better readability and usability.
@sogko sogko merged commit 474a9cf into graphql-go:master Nov 18, 2015
@sogko
Copy link
Member

sogko commented Nov 18, 2015

I'll go ahead and merge this, so that we can facilitate other PRs to move forward.

Thanks again @Fugiman 👍🏻🌟

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.

3 participants