Skip to content

Large re-organization to remove cyclic module dependency graph #381

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 18 commits into from

Conversation

chrisdone
Copy link
Member

Related to #187. The current master module hierarchy evolved organically with much sharing between files, making it a mess of a graph (see @hvr's picture). This pull request finally corrects that. Opening as a pull request because any changes you guys have committed locally will have to be rebased. So I suggest you push your changes first and I will rebase upon them. However, the longer this goes unmerged it will be harder to do so. I'm going to give it a grace period of a week before merging, although if everyone is happy to go ahead I'll merge it sooner.

I wrote a trivial script that would rm all .elc files and byte-compile just the file I wanted to test. Using that, I ran it on every file in the project to see that every file had the proper require dependencies. Naturally, the HIM modules were all an interloping mess, so I've significantly moved code around so that the hierachy fits. Here is the new hierarchy:

deps

As you can see, at the top is the haskell module. Given that the haskell-mode.el module contains the mode, it didn't make sense to have everything being pulled into there, especially given that many things depend on haskell-mode.el itself. So we end up with three 'big' modules that are depended on a lot: haskell-mode.el, haskell-interactive-mode.el and haskell-process.el. I removed all the specialized commands and things from haskell-session, haskell-process and haskell-interactive-mode into haskell-load or haskell-commands. It makes those modules nice and small (relatively speaking). I would like to do further breaking down of that haskell.el file until it's nearly just a list of imports.

I didn't actually rename any functions, though I would've liked to (using make-obsolete), but that would be far more problematic. The point is to move things in the right files. It's not entirely finished, nor am I sold on the particular module naming, but I consider this a big step in the right direction. After this we can incrementally improve.

@purcell
Copy link
Member

purcell commented Nov 29, 2014

Looks really good to me, but I feel sorry for the #370 guy, because he's going to have to completely rework his PR (again) if it doesn't get merged before this one. Would it be a pain for you to integrate his changes?

@chrisdone
Copy link
Member Author

It looks like his PR is mergeable at this point? I'm not a Nix user so I can't confirm it. At any rate I don't intend on making anyone else re-do work.

@purcell
Copy link
Member

purcell commented Nov 30, 2014

Yes, I'd consider it basically mergeable, but have reservations about the hack to install el-mock. If it's okay with you (at least for now), then we can go ahead.

@chrisdone
Copy link
Member Author

Agreed, last time someone added Cask to one of my repositories I never finished getting it to install and have never used it since. Let's merge without the tests and move that elsewhere.

@chrisdone
Copy link
Member Author

Grace period over. Merged!

@chrisdone chrisdone closed this Dec 6, 2014
@purcell
Copy link
Member

purcell commented Dec 6, 2014

Great job!

ikirill pushed a commit to ikirill/haskell-mode that referenced this pull request Dec 28, 2014
@gracjan
Copy link
Contributor

gracjan commented Mar 6, 2015

@chrisdone: Is your module-refac branch still needed or can we remove it?

@gracjan gracjan deleted the module-refac branch March 20, 2015 10:46
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