Skip to content

merged hi2 (https://github.com/errge/hi2). #483

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

Closed
wants to merge 1 commit into from
Closed

merged hi2 (https://github.com/errge/hi2). #483

wants to merge 1 commit into from

Conversation

kuribas
Copy link
Contributor

@kuribas kuribas commented Feb 27, 2015

Merge hi2.el changes with haskell-indentation

@kuribas
Copy link
Contributor Author

kuribas commented Feb 27, 2015

Why does it complain about CL functions?

@gracjan
Copy link
Contributor

gracjan commented Feb 27, 2015

@kuribas: use cl-find-if, cl-remove-if and friends. It was decided (long time ago) to use cl-lib.el instead of cl.el proper so cl functions get the prefix. I guess there is no need to fight this particular decision.

@purcell
Copy link
Member

purcell commented Feb 27, 2015

use cl-find-if, cl-remove-if and friends.

Yes please.

Also, can we take this opportunity to prefix the unprefixed top-level symbols such as following-token?

Apart from that, as a hi2 user I'm very much +1 on merging this, and will do so when the above changes are done unless I hear otherwise from the other maintainers.

@gracjan
Copy link
Contributor

gracjan commented Feb 27, 2015

@purcell: Can we MELPA stable before merging, as this is potentially shockingly great change but a bit involved one?

@purcell
Copy link
Member

purcell commented Feb 27, 2015

@gracjan What do you mean by "can we MELPA stable"?

@gracjan
Copy link
Contributor

gracjan commented Feb 27, 2015

@purcell: Tag a stable release before merging hi2.

@purcell
Copy link
Member

purcell commented Feb 27, 2015

Perhaps. When it's been so long since the last numbered release, I doubt that this would be the change that would cause the most breakage if people suddenly start getting a new version with much more recent code: the whole haskell-process/haskell-interactive-mode stuff would be the big issue. In the meantime, we should feel free to apply to master reasonable changes, and be prepared to quickly fix any breakage.

@gracjan
Copy link
Contributor

gracjan commented Mar 1, 2015

@kuribas, @purcell: this hi2 is super cool stuff!

I fixed the missing cl-xx and then it works as a charm.

@purcell
Copy link
Member

purcell commented Mar 1, 2015

this hi2 is super cool stuff!

Great, isn't it? :-)

@purcell
Copy link
Member

purcell commented Mar 1, 2015

Am I right in thinking this now needs to be rebased against master?

@gracjan
Copy link
Contributor

gracjan commented Mar 1, 2015

I merges cleanly so rebase should be no problem.

@purcell
Copy link
Member

purcell commented Mar 1, 2015

@gracjan Where are your fixes for remove-if etc?

@gracjan
Copy link
Contributor

gracjan commented Mar 1, 2015

Local only.

@errge
Copy link
Contributor

errge commented Mar 1, 2015

Hey Guys,

I just wanted to say that I'm so grateful and happy that @kuribas took this up and is converting my messy hi2 to something that can be contributed back to the original codebase! You rock!

It was obviously not my intention to fork haskell-mode, just was annoyed with the indentation stuff and had the idea of having some visual feedback and wanted to practice my elisp skills on a hackathon. After that, reality checked in and I have never found the time to properly contribute back my changes to the community.

So a big thank you for everyone involved and especially to @kuribas. My work would not have been possible without his haskell-indentation parser and also would have never been finished without his finishing touches!

Once this is merged, please feel free to send a pull request against hi2 that warns the user that my hack is not needed anymore!

Cheers,
Gergely


;; Author: Kristof Bastiaensen <[email protected]>

;; Author: Gergely Risko <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use [email protected] with this merge? I know that I've used [email protected] myself, but I actually prefer [email protected] for haskell projects nowadays!

Conflicts:
	haskell-indentation.el
@kuribas
Copy link
Contributor Author

kuribas commented Mar 1, 2015

@errge thanks, but it wasn't that much work to integrate it, and I really like the changes you made.

@gracjan
Copy link
Contributor

gracjan commented Mar 3, 2015

I gave it a couple of days of usage and I think the plan to getting hi2 merged in should be:

  • create a branch here so that we can work on this together
  • create a test suite so that from the get-go we care about quality
  • fix a couple of remaining surprises (probably caused by merge)
  • fix a couple of 'debugger entered' situations

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

@gracjan If you tell me the bugs, or file an issue on my repository, I'll try to fix it. A test suite is a good idea. I have been thinking to run it automatically on a large haskell source file (ghc maybe?), preferably containing lots of syntax extensions. It could then report where the parser fails.

@gracjan
Copy link
Contributor

gracjan commented Mar 3, 2015

I'll try to create a test suite out of this, for now as a teaser for you issues I found:

http://pastebin.com/ypr4SajS

(Note that this pertains to your merge rebased over current master).

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

I made some comments: http://pastebin.com/gU8pr6Tu

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

Note that I consider it a bug only when:

  • it throws a parseerror on valid haskell
  • it fails to show a valid indentation (or a semantically equivalent one).
  • it has inconsistent indentation (debatable in some cases)

Everything else is a feature :-)

In theory I'd like it to support all GHC syntax extensions, but in practice people need to file a bug report when they need support for the extension.

@gracjan
Copy link
Contributor

gracjan commented Mar 3, 2015

@kuribas:

  1. Underscore bugs are due to change of interpretation of underscore, this is now 'symbol char' when it used to be 'word char'. c-mode/java-mode define it like this and we should be compatible.
  2. You have seen the bugs, they were mostly the annoying ones.
  3. val :: String <- return "sdfa" is an extension, I forgot which one was it.

I guess we need 1. and 2. before merging. Other stuff is lower priority.

Debugger jumps on me when there is something wrong with the code a screen or two above (invisible). It is pretty annoying. I think that indentation should do something predictable given any garbage as input. That would also ensure that possible future haskell extensions are compatible with haskell-indentation.

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

I don't think there is any need to support garbage, but I agree that giving meaningful feedback when the parser fails would be a good idea.

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

Btw, the parser looks at the code before point to see how to indent it. It considers all code from the closest line before point that has zero indentation. So if there is any garbage there, it will crash with a parse error.

@gracjan
Copy link
Contributor

gracjan commented Mar 3, 2015

@kuribas: 'Support' in the minimal sense of not doing anything super surprising, like messing up whole buffer, hanging or popping up debugger. TAB should not cause damage. TAB should also not cause interactive features.

@kuribas
Copy link
Contributor Author

kuribas commented Mar 3, 2015

@gracjan Does it do anything like that? It shouldn't even enter the debugger for a parser error, since it uses condition-case.

@gracjan
Copy link
Contributor

gracjan commented Mar 3, 2015

@kuribas: does for me.

I do have (debug-on-error . t) but then it should not matter as docs say:

Does not apply to errors handled by condition-case' or those matched by debug-ignored-errors'.

Aquamacs 3.2 GNU Emacs 24.4.51.2 (x86_64-apple-darwin14.0.0, NS apple-appkit-1343.14) of 2014-11-07 (Aquamacs-3.2) on watson.local

@gracjan
Copy link
Contributor

gracjan commented Mar 6, 2015

I've created a branch called 'wip/hi2' that is rebased on master, has cl-xx functions fixed and documents one failing test case (for now). @kuribas: can you have a look at it?

@gracjan
Copy link
Contributor

gracjan commented Mar 9, 2015

@kuribas: I've reported an issue nilcons/hi2#15 in @errge repo.

I think that when those two are fixed we can merge!

@gracjan
Copy link
Contributor

gracjan commented Mar 9, 2015

@kuribas: So I fixed those myself.

@gracjan
Copy link
Contributor

gracjan commented Mar 12, 2015

Surprises fixed on wip/hi2, and the 'debugger entered' issues was because of my (debug-on-error t).

@gracjan gracjan closed this Mar 14, 2015
@gracjan
Copy link
Contributor

gracjan commented Mar 14, 2015

Closing in favor of #502.

@gracjan
Copy link
Contributor

gracjan commented Mar 18, 2015

@kuribas: does hi2 reparse current function at every cursor movement? Including left-right? It is slow.

@kuribas
Copy link
Contributor Author

kuribas commented Mar 19, 2015

Yes, because it uses pre-command and post-command hooks. The original mode only parsed after a tab or delete.

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.

4 participants