Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Handwrite ToJSON instances, fix #32 #35

Merged
merged 1 commit into from
Nov 8, 2015

Conversation

cocreature
Copy link
Collaborator

No description provided.

@gracjan
Copy link
Contributor

gracjan commented Nov 7, 2015 via email

@alanz
Copy link
Collaborator

alanz commented Nov 7, 2015

@gracjan put forward suggestions for field names, or a PR

@cocreature
Copy link
Collaborator Author

@gracjan Since the json is not actually meant for human consumption (taking out the fact that you probably need to read it during development). I would prefer keeping it as close as possible to the names in the Haskell source. Otherwise it gets harder to figure out where something is coming from which makes debugging harder and in the case where there is no documentation (which happens :)) you also can't look it up easily in the source.

@alanz
Copy link
Collaborator

alanz commented Nov 8, 2015

I tend to agree with @cocreature on this. I do understand there could be an argument for the field names to map more cleanly into the s/w on the other side of the API, but I am not sure of the benefits or what a reasonable naming scheme is.

@gracjan
Copy link
Contributor

gracjan commented Nov 8, 2015

Than at least please make naming consistent. Currently we have cmd (short version), HieError (camel case), contents (full, no prefix), ctxCabal (with prefix, full, camel case).

I'd like to propose rules for converting internal names to external that match json conventions:

  • prefix is removed
  • camelCase is converted to underscores (underscore_case)
  • use full names for consistency
  • start with lowercase letter

Examples:

  • cmd => command
  • ctxCabal => cabal (note: the could be renamed to ctxCabalFilePath and cabal_file_path to better state what it is)
  • HieError => hie_error

And to address @cocreature points: external protocol is very important so that plugin developers find it easy to integrate. The key people to make HIE a success are developers reading the raw protocol exchanges. Note that I'm talking about editor integrations developers, they will not really read Haskell source code. It sees that you want to make it easy for yourself at their expense, I think this is wrong cost allocation.

Note that external protocol must be much more stable than internal data types. I can fully imagine that some day we will refactor everything on the Haskell side, but still keep the external protocol.

@alanz
Copy link
Collaborator

alanz commented Nov 8, 2015

While we are on the topic of external interface stability, @gracjan and I discussed on IRC removing Context completely, and replacing it with a set of well-known parameter names.

The concept of a context exists as something to be sent to the IDE as an attribute of a command, so the IDE can make it available in that context and make sure that the required parameters are returned.

So perhaps this change should come in, which simplifies/reduces the number of JSON instances required. Or should it be discussed in a completely separate place?

@gracjan
Copy link
Contributor

gracjan commented Nov 8, 2015

@alanz: I was thinking about very similar change. Technically Emacs will give all available context to every command because there is no need to not give it, if it is available.

@alanz
Copy link
Collaborator

alanz commented Nov 8, 2015

OK, will do so. Unless anyone else wants to do it now, will get on it in about half an hour.

@gracjan
Copy link
Contributor

gracjan commented Nov 8, 2015

I will not be able to touch it today.

@alanz
Copy link
Collaborator

alanz commented Nov 8, 2015

In terms of this PR, there are two options

  1. Land it as is, then @alanz removes the Context from the IdeRequest, then we rename external names as proposed by @gracjan
  2. Ask @cocreature to make the external name changes, then land it. Context removal to happen afterwards.

Thoughts?

@cocreature
Copy link
Collaborator Author

updated

alanz added a commit that referenced this pull request Nov 8, 2015
@alanz alanz merged commit 5675072 into haskell:master Nov 8, 2015
@cocreature cocreature deleted the json-instances branch November 26, 2015 19:30
@alanz alanz added this to the prehistory milestone Feb 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants