Move functions to inc/ and add more tests #196

lucc · 2016-06-29T10:09:21Z

@rkitover can you also test this on non linux system please. Especially the "Unify ..." commits.

Use tr -d '\015' to remove CRs instead of sed 's/\r//', many versions of sed do not understand \r.

In quit() for both vimpager and vimcat, change `exit "$@"` to `exit "${@:-0}"` so that the default is zero explictly. For some reason on OpenBSD `quit` for vimpager was returning 1.

rkitover · 2016-06-29T20:05:36Z

@lucc I fixed a couple of minor issues and rebased, now it passes on OS X, Sol10, FreeBSD and OpenBSD.

I haven't read the changes yet, will do so soon.

Could we please default to 4 space indent in new work? Just because that's what we made the main files. If you really need the two space indent it's ok I don't really care that much, but just trying to be consistent as much as possible.

lucc · 2016-06-29T20:42:24Z

Ok will do the 4 spaces thing.

Is there a reason you put the github issue number in the git commit message? I would remove that.

lucc · 2016-06-29T20:47:37Z

inc/vimcat_functions.sh

@@ -19,7 +19,7 @@ quit() {

        rm -rf "$tmp" 2>/dev/null # rm -rf "" shows error on OpenBSD
    ) &
-    exit "$@"
+    exit "${@:-0}"


I will also change this to exit "${1:-0}" as e.g. exit 1 2 fails. Some interactive shells don't even exit if you give more than one argument to exit.

rkitover · 2016-06-29T20:56:10Z

@lucc good point, any idea why the travis build isn't running for this branch?

lucc · 2016-06-29T20:58:08Z

yes: https://www.traviscistatus.com/

I canceled the old osx builds that were blocking the current linux build. osx might still take a while and we could cancel some of them to see the linux builds run.

lucc · 2016-06-29T23:05:34Z

@rkitover are you manually restarting builds on travis? I canceled some but their status changed to "created" again (was "canceled" before).

rkitover · 2016-06-30T00:57:03Z

@lucc nope...

rkitover · 2016-06-30T17:53:57Z

@lucc all of this looks great, let me know when you want it merged.

That said, I'd like to do some things a little bit differently. I'm fine with doing this later and merging this now or doing it before we merge, either way:

let's leave program specific functions in the program, and just set a variable to not call main() when we source it for the functions, e.g.

[ "${VIMPAGER_SOURCE:-0}" -ne 0 ] && main "$@"

in this way, the functions can be tested without invoking the program, or reused, or whatever.

The intent of this is to have at least the relevant code in the program itself for people to look at.

runtime shell code should go into sh/ while stuff that gets included should go into inc/. Neither of these dirs are used by vim so we don't break the plugin bundle functionality.
common functions should be called something like sh/lib.sh
sh/ should get installed sh/lib.sh should be sourced at runtime by the installed version and the git version

The intent of the sh/ directory is to share shell code, as well as perhaps to spin off some of our work into separate projects at a later time. Like I was thinking of creating a platform.sh for shell programmers at some point with some of this stuff. Certainly I want to spin off my uudecoding work, that stuff was a lot of work, and it could be useful for things that make portable shell archives (I forget right now what that project was called...)

That's all I have. Questions, comments, disagreements? Let's discuss.

I am going to be traveling the next few days, my main computer with all of my VMs will be packed up (it's a 2011 mac mini) but I will have my laptop to check on things on the road. We're driving from the east coast of the US to the west coast, from Philadelphia to Los Angeles, and then we'll be on a 3 day cruise (where I'll have some limited internet and my laptop too.)

So I will be checking in on the project but can't do OS testing for at least a few days.

rkitover · 2016-06-30T22:06:50Z

@lucc also, just a reminder, you were going to give me more details about the hang you get with dash on arch. As I told you I made an arch VM and was not able to reproduce the problem.

lucc · 2016-06-30T23:14:35Z

sh/lib.sh could also be lib/functions.sh or lib/vimpager.sh or so. I would prefer lib/ over sh/ as it is closer to things like /usr/lib/ in name.

I think installing code into /usr/local/lib is good. But when installing these "source files" we need to give them a better name than lib.sh or functions.sh as these are way to generic for a system wide lib directory.

I would also be fine with just renaming inc/ to lib/ (moving all files from inc/ to lib/). That would make it more uniform. Obviously only the needed files should be installed by make install.

I am fine with keeping some functions in the main scripts. But I think introducing a new environment variable just to not run main is strange. I suggest to move all functions that we want and can test to the separate files in inc/ and lib/ . These files should be sourceable without side effects aside from function and variable definitions. main() and maybe a few other functions would stay in the main scripts to give readers an idea about the code.

Factoring out code into a separate project: don't care.

We can either rebase and edit this branch or add new commits to do it. We can also merge now or add these commits first, I don't care.

I also AFK from time to time, no worries. (for example this weekend)

PS: I am running irc://luc.now-ip.net/#chat on my raspberry

lucc · 2016-07-06T08:47:46Z

I will put together a report about the nvim+dash+man problem. It is dependent on my vimrc file so it is probably only a problem for me. Currently I just install vimpager with POSIX_SHELL=/bin/bash to solve it.

After some thought: lets merge this now/soon and do any further changes in a new PR. That makes it easier to start new PRs as this one is a big change that would result in merge conflicts with any other work.

lucc · 2016-09-02T14:04:06Z

Bump: rebased after recent changes in master.

rkitover · 2016-09-04T07:36:31Z

Hey @lucc,

sorry for the long absence, but I just moved to my mom's house on the other coast of the US (San Francisco from Philadelphia, passing through Los Angeles) for a few months, and ended up doing a clean install of my mac etc. so I didn't have all my stuff to work with.

Anyway, I got my VMs back up, still need to fix some issues with the mac like the unstable thunderbolt GPU, but I can get back into things slowly.

Will get back to you on this soon.

lucc · 2017-02-10T17:22:19Z

The tests do not yet pass but I managed to rebase it. I think the test failure is due to a recent change in the vimrc searching code.

lucc · 2017-02-11T10:26:45Z

The test for find_vimpagerrc_files fails because of the changes in 1e862d5. Do would you like me to remove the test or try to fix it (it looks like the function can no longer garante that it will set these variables so what could we test?)

lucc · 2017-02-14T19:39:54Z

I removed the offending test for now. I still have it locally so I will try to add it later again, but I want to get this PR finished some day, so not here :)

The project language is set to "generic" because the ruby specific setup is not needed.

lucc · 2017-02-20T13:42:37Z

On travis only the osx tests fail because of some strange issue with script and the tty.

Use tr -d '\015' to remove CRs instead of sed 's/\r//', many versions of sed do not understand \r.

In quit() for both vimpager and vimcat, change `exit "$@"` to `exit "${1:-0}"` so that the default is zero explicitly and quit() doesn't fail if called with more than one argument. For some reason on OpenBSD `quit` for vimpager was returning 1.

lucc · 2017-02-20T13:54:25Z

Same solution as above: Just don't build on osx for now (that was very slow to start as well). I still have the code locally so we can figure it out later.

lucc mentioned this pull request Jun 29, 2016

Add a test suite. #194

Merged

lucc force-pushed the test2 branch 3 times, most recently from 7a15f49 to cfa817a Compare June 29, 2016 14:37

rkitover added a commit that referenced this pull request Jun 29, 2016

remove CRs more portably in tests #196

9eae627

Use tr -d '\015' to remove CRs instead of sed 's/\r//', many versions of sed do not understand \r.

rkitover force-pushed the test2 branch from 9eae627 to 3f36dc2 Compare June 29, 2016 19:47

rkitover added a commit that referenced this pull request Jun 29, 2016

remove CRs more portably in tests #196

3f36dc2

Use tr -d '\015' to remove CRs instead of sed 's/\r//', many versions of sed do not understand \r.

lucc reviewed Jun 29, 2016
View reviewed changes

lucc force-pushed the test2 branch 2 times, most recently from bef454f to dea3ae1 Compare June 29, 2016 22:13

lucc force-pushed the test2 branch 2 times, most recently from 126deaa to dea3ae1 Compare June 30, 2016 00:25

lucc force-pushed the test2 branch from 88a761b to 5127637 Compare June 30, 2016 08:37

This was referenced Jul 8, 2016

vimrc specific hang with neovim+dash+man #197

Open

Add basic tests #180

Open

lucc mentioned this pull request Jul 19, 2016

factor out and improve common code in vimpager/vimcat #182

Open

lucc force-pushed the test2 branch from 5127637 to 772cf88 Compare September 2, 2016 14:03

lucc force-pushed the test2 branch from 772cf88 to d188ce5 Compare September 13, 2016 06:48

lucc mentioned this pull request Feb 10, 2017

Should we tag a release? #210

Open

lucc force-pushed the test2 branch from d188ce5 to 1463760 Compare February 10, 2017 17:21

lucc force-pushed the test2 branch from 1463760 to 3118822 Compare February 14, 2017 19:25

lucc added 14 commits February 20, 2017 14:10

Move all function definitions to inc/

5fc412e

Add some tests for vimpager shell functions

df7c031

Add some tests for vimcat shell functions

79d7bbc

Move function common to vim{pager,cat} to a seperate file

4dfd5ef

Add test for functions common to vim{pager,cat}

7c07c6e

Add simple test for vimpager output

823d225

Move further code from vimpager into functions

2fda452

Move further code from vimcat into functions

6e6de15

Rename $tmp_dir to $tmp in vimcat

c7179f6

Unify create_tmp_directory()

04d74c6

Unify system detection

ec397c7

Unify find_tmp_directory()

110cb36

Add test for set_system_vars()

c9db96b

Set language to generic for travis

ea1240a

The project language is set to "generic" because the ruby specific setup is not needed.

lucc force-pushed the test2 branch from 3118822 to d66e1b3 Compare February 20, 2017 13:10

rkitover and others added 4 commits February 20, 2017 14:44

remove CRs more portably in tests

ade017b

Use tr -d '\015' to remove CRs instead of sed 's/\r//', many versions of sed do not understand \r.

fix weird test fail on OpenBSD

6f4f74a

In quit() for both vimpager and vimcat, change `exit "$@"` to `exit "${1:-0}"` so that the default is zero explicitly and quit() doesn't fail if called with more than one argument. For some reason on OpenBSD `quit` for vimpager was returning 1.

Test make install on travis

6e99b90

Clean up sed commands in the makefile

a8648e6

lucc force-pushed the test2 branch from d66e1b3 to a8648e6 Compare February 20, 2017 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move functions to inc/ and add more tests #196

Move functions to inc/ and add more tests #196

lucc commented Jun 29, 2016 •

edited

Loading

rkitover commented Jun 29, 2016

lucc commented Jun 29, 2016

lucc Jun 29, 2016

rkitover commented Jun 29, 2016

lucc commented Jun 29, 2016

lucc commented Jun 29, 2016

rkitover commented Jun 30, 2016

rkitover commented Jun 30, 2016

rkitover commented Jun 30, 2016

lucc commented Jun 30, 2016

lucc commented Jul 6, 2016

lucc commented Sep 2, 2016

rkitover commented Sep 4, 2016

lucc commented Feb 10, 2017

lucc commented Feb 11, 2017

lucc commented Feb 14, 2017 •

edited

Loading

lucc commented Feb 20, 2017

lucc commented Feb 20, 2017

Move functions to inc/ and add more tests #196

Are you sure you want to change the base?

Move functions to inc/ and add more tests #196

Conversation

lucc commented Jun 29, 2016 • edited Loading

rkitover commented Jun 29, 2016

lucc commented Jun 29, 2016

lucc Jun 29, 2016

Choose a reason for hiding this comment

rkitover commented Jun 29, 2016

lucc commented Jun 29, 2016

lucc commented Jun 29, 2016

rkitover commented Jun 30, 2016

rkitover commented Jun 30, 2016

rkitover commented Jun 30, 2016

lucc commented Jun 30, 2016

lucc commented Jul 6, 2016

lucc commented Sep 2, 2016

rkitover commented Sep 4, 2016

lucc commented Feb 10, 2017

lucc commented Feb 11, 2017

lucc commented Feb 14, 2017 • edited Loading

lucc commented Feb 20, 2017

lucc commented Feb 20, 2017

lucc commented Jun 29, 2016 •

edited

Loading

lucc commented Feb 14, 2017 •

edited

Loading