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

Rename 'Values' to 'Const' #50

Closed
jml opened this issue Jan 2, 2017 · 6 comments
Closed

Rename 'Values' to 'Const' #50

jml opened this issue Jan 2, 2017 · 6 comments

Comments

@jml
Copy link
Collaborator

jml commented Jan 2, 2017

Or "Constant". Or "Constants". We talked a bit about this in #19. The thing about our "Values" module is that it only refers to constants, not variables. This is a super useful distinction that becomes more obvious when looking at validation code.

Accompanying change should consider changing ToValue and FromValue to also use "constant" (or whatever) in the name.

@jml
Copy link
Collaborator Author

jml commented Jan 2, 2017

@teh thoughts?

@teh
Copy link
Collaborator

teh commented Jan 3, 2017

I'm still not convinced that's a good idea name-wise (like with literals). The distinguishing feature of Values IMO is that they are dynamically typed, not that they are constant?

I may be too old-school but for me constants and literals are values that are typed directly into the program code, e.g. like in http://www.cplusplus.com/doc/tutorial/constants/ - skimming the spec I think graphql thinks about constants similarly (e.g. in me(arg: string = "hello") "hello" is a const)

There are other traditions (like physics) where constants are forever-fixed values (like ℏ).

The spec also talks about "values" (input value, float values).

Maybe I'm misunderstanding what you are proposing?

@jml
Copy link
Collaborator Author

jml commented Jan 3, 2017

distinguishing feature of Values IMO is that they are dynamically typed

That distinguishes them from Haskell code, but not from the rest of the GraphQL AST.

constants and literals are values that are typed directly into the program code

Literals are that. Constants are more complicated.

e.g. 3.1416 is a literal. const float pi = 3.1416 defines a constant. pi is a constant.

graphql thinks about constants similarly (e.g. in me(arg: string = "hello") "hello" is a const)

Yes, but in the following, me(arg: string = $greeting), $greeting is a variable and also a value (both in spec terms -- https://facebook.github.io/graphql/#Value -- and in parser terms https://github.com/jml/graphql-api/blob/master/src/GraphQL/Internal/AST.hs#L189). When we do variable substitution, it becomes me(arg: string = "hello"), where "hello" is a non-variable value. Having a term for "values that aren't variables" that isn't "Value" would be very useful. GValue.Value is definitely not self-explanatory.

Note also that the spec uses "Const" as a qualifier on the kinds of Value, and uses that qualifier to restrict what a Default value can be (see https://facebook.github.io/graphql/#DefaultValue).

All that said, I don't want to convince you against your will. If you find a proposed name misleading, we shouldn't use it.

@teh
Copy link
Collaborator

teh commented Jan 3, 2017

From reading I'm not sure we're talking about the same issue but I'm also not sure where the misunderstanding happens. I'm thinking about e.g. BooleanValue here - would you rename that to BooleanConst?

Maybe let's do this IRL :)

@jml
Copy link
Collaborator Author

jml commented Jan 3, 2017

I'm talking about renaming:

  • GraphQL.Value to GraphQL.Const
  • GraphQL.Value.Value to GraphQL.Const.Const
  • no strong opinions about constructor names

@jml
Copy link
Collaborator Author

jml commented Jan 14, 2017

#66 does enough of this to keep me happy.

@jml jml closed this as completed Jan 14, 2017
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