Skip to content

Add tests to haskell-process-wrapper-function #491

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

Conversation

ardumont
Copy link
Contributor

@ardumont ardumont commented Mar 3, 2015

@cocreature
Copy link
Contributor

As this is no longer related to cask, you should probably change the commit message. What is the reason for changing the gitignore? Is that no longer needed?

@ardumont
Copy link
Contributor Author

ardumont commented Mar 3, 2015

lol.

Indeed, I focused so much on the content of the commit, I forgot the message!

For the .gitignore, I tried to keep it it was before.

@ardumont ardumont force-pushed the add-tests-on-haskell-process-wrapper-function branch from 6765c03 to 7bcb572 Compare March 3, 2015 21:26
@ardumont
Copy link
Contributor Author

ardumont commented Mar 3, 2015

Rebased!

Thanks!

@ardumont ardumont mentioned this pull request Mar 3, 2015
@ardumont ardumont force-pushed the add-tests-on-haskell-process-wrapper-function branch 3 times, most recently from 4a6a78b to 50585eb Compare March 3, 2015 22:37
@ardumont ardumont force-pushed the add-tests-on-haskell-process-wrapper-function branch from 50585eb to 07bf9cd Compare March 3, 2015 22:49
(package-initialize)
(add-to-list 'package-archives '("melpa" . "http://melpa.milkbox.net/packages/"))
(package-refresh-contents)
(package-install 'el-mock))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As usual, beware the ugly hack!

Uglier because of emacs-23 compat' issue on package.

@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

@ardumont: There is a directory tests/compat that already has cl-lib.el and ert.el. Just put el-mock.el in there and you will not need to download it from anywhere.

(el-mock.el is under GPL and has license in itself, so this copy is ok license wise).

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

Neat, way better!

Thanks for the information, will update this then.

Cheers,

On Wed, 4 Mar 2015 09:01 gracjan [email protected] wrote:

@ardumont https://github.com/ardumont: There is a directory tests/compat
that already has cl-lib.el and ert.el. Just put el-mock.el in there and
you will not need to download it from anywhere.

(el-mock.el is under GPL and has license in itself, so this copy is ok
license wise).


Reply to this email directly or view it on GitHub
#491 (comment).

@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

Having said that can't we get away without el-mock.el? For example this fragment:

(mocklet (((haskell-session-name "dummy-session") => "dumses1"))
    (haskell-process-compute-process-log-and-command "dummy-session" 'ghci)))

isn't it equivalent to:

(let ((haskell-session-name (lambda (x) "dumses1")))
    (haskell-process-compute-process-log-and-command "dummy-session" 'ghci)))

?

Emacs is dynamically bound (usually). This has plenty of downsides but a couple of upsides, too :)

@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

Also cl is frowned upon in favor of cl-lib.

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

(let ((haskell-session-name (lambda (x) "dumses1")))
(haskell-process-compute-process-log-and-command "dummy-session" 'ghci)))

I'm not sure.
Also, I'm not sure that the different emacs versions does work the same way with this.

Emacs is dynamically bound (usually). This has plenty of downsides but a couple of upsides, too :)

Indeed.

Also cl is frowned upon in favor of cl-lib.

Right. I admit, I'm kind of overwhelmed with this cl thing.
I never remember which one to use :D

@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

Digging inside el-mock.el reveals that it does some advanced redefinition magic and does not use dynamic variables. It that case it makes sense to pull this dependency in.

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

Digging inside el-mock.el reveals that it does some advanced redefinition magic and does not use dynamic variables. It that case it makes sense to pull this dependency in.

I'm at work right now.
This evening, I will rework this PR to add el-mock.el in the tests/compat/ folder
(EDIT: fix from typo contrib to tests/compat).

As a consequence, I'll also rework #492 and remove the dependency on this PR (that is by removing the common commit, this one).

Cheers,

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

Nope, that won't work.

The tests/compat folder is for emacs version < 24.
We need el-mock for all versions (as per this PR).

What about adding another folder tests/deps (or a better name) and adding el-mock.el there.
Then updating the EFLAG entry to add this folder as dependencies?

@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

test/compat should be added as the last element to load-path so that whatever is not found in standard locations is found there.

And it should be added for all Emacs versions.

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

Nope, that won't work.

And indeed, it does not.

What about adding another folder tests/deps (or a better name) and adding el-mock.el there.
Then updating the EFLAG entry to add this folder as dependencies?

This does :D

test/compat should be added as the last element to load-path so that whatever is not found in standard locations is found there.
And it should be added for all Emacs versions.

Are you suggesting I update the EFLAG according to this and integrate this in this PR?

@ardumont
Copy link
Contributor Author

ardumont commented Mar 4, 2015

test/compat should be added as the last element to load-path so that whatever is not found in standard locations is found there.
And it should be added for all Emacs versions.

Are you suggesting I update the EFLAG according to this and integrate this in this PR?

It also works (at least on my emacs-24-4).

@ardumont ardumont force-pushed the add-tests-on-haskell-process-wrapper-function branch from b8e1789 to 18db008 Compare March 4, 2015 10:24
@gracjan
Copy link
Contributor

gracjan commented Mar 4, 2015

@ardumont: looks very good! I'll check this out soon as time permits.

@gracjan
Copy link
Contributor

gracjan commented Mar 8, 2015

I've reworked your commits for better history and merged them. Thanks!

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

gracjan commented Mar 8, 2015

@ardumont: note that there are plenty of other functions in haskell-process that could use unit testing. You have el-mock.el in the repo, how about you write some more tests?

@ardumont
Copy link
Contributor Author

ardumont commented Mar 9, 2015

I've reworked your commits for better history and merged them. Thanks!

Great!

@ardumont: note that there are plenty of other functions in haskell-process that could use unit testing. You have el-mock.el in the repo, how about you write some more tests?

Right. I'll see what I can do.

Cheers,

@ardumont ardumont deleted the add-tests-on-haskell-process-wrapper-function branch March 9, 2015 07:12
@ardumont
Copy link
Contributor Author

ardumont commented Mar 9, 2015

I'll see what I can do.

WIP https://github.com/ardumont/haskell-mode/tree/add-tests-to-haskell-process-namespace

I'm working (slowly) on it.

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

Successfully merging this pull request may close these issues.

3 participants