search

rkitover / vimpager

Use Vim as PAGER
http://www.vim.org/scripts/script.php?script_id=1723
Other
776 stars 72 forks source link

Questions about implementation details. #149

Open lucc opened 8 years ago

lucc commented 8 years ago

This is a random collection of questions that I came up with when I read through the code in my attempt to create a PR as discussed in #58. They are mainly questions about the intention and reason of certain parts of the code.

  1. Which shells are suported? Can we have a comment in the source about where to find documentation on them? I find it hard to work on the shell script which tries to do the right thing for so many different shells without having at least a hint what shells these are. Maybe the shells can be stated in the Readme as runtime dependencies as well.
  2. Is it (in light of the trouble of supporting many differnt shells) desireable to move logic from the shell script into vimscript?
  3. Is it desirable to move vimscript code from --cmd and -c options into autoload files in order to clean up the shell script?
  4. Regarding vim script: Which versions of vim do you want to support?
  5. vimpager uses vim -c ':args 'file1 file2 ..' to pass the files to vim (line 834). Is there a special reason why they are not listed as ordinary arguments?
  6. Both syntax versions for command substitution are used. Should that be changed to only one? At least after the shell selection hack?

Maybe more later ...

rkitover commented 8 years ago

Hi @lucc,

  1. vimpager and vimcat are written for POSIX sh, the canonical reference is here: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_01 in practice that means shells like ksh and ash are the best shells to test with. POSIX sh means not using bash features like arrays.
  2. I try to do this when I can now, but sometimes the interface functions better when some work is done with shell tools.
  3. I've done most of this already if you look at master.
  4. 7.3+ for now
  5. suppressing modeline errors, perhaps there's a better way to do this
  6. $(...) is fine as it's POSIX
lucc commented 8 years ago

I just thought about the shell selection: If we install vimpager into /usr/local via the makefile we could also select a shell in the makefile and hard code it into the script. Then the shell selection hack would only be neccessary for the portable version. And also the system selection could be moved to the makefile and the portable version (have a look at pass they insert OS specific code into the script when installing).

rkitover commented 8 years ago

That's a good idea, I will do this now. With one caveat, the version in the repo has to run it for pathogen users.

lucc commented 8 years ago

You can have a look at the code in the script and in the makefile of pass. Like this you can run the script from the git repository without bundleing anything. And platform specific code can be sourced into the main script (you might have noticed that I advocate to put extra code into the script during installation instead of removing unneeded code during installation, I think it is easier and less error prone).

rkitover commented 8 years ago

That's sort of what I was planning on doing.

rkitover commented 8 years ago

@lucc Ok, I've implemented this, looks ok to you?

lucc commented 8 years ago

Question 7: What make versions are supported? Is there a reason that you write all shell commands in the makefile seperated by semicolon+backslash+newline instead of putting them on differnt lines (apart from the cases where it is neccessary for shell control structures (if, while, ...) and shell variables)?

About af50ec9: I started testing. The first thing I noticed is that the order of the shell you check changed and now includes dash, ash and zsh. I have dash and zsh installed and have some trouble with dash: PAGER=./vimpager man man tries to use dash but hangs. ./vimpager file works (also uses dash). If I uninstall dash vimpager uses bash and also works with man.

I installed version 2.06.r141.gaf50ec9 from the AUR and it uses bash. That is strange and I have to investigate some more.

But I had an idea for a --version option. See my PR. This might help when discussing issues as we can be sure what we talk about.

rkitover commented 8 years ago

The make I target is standard UNIX make, or POSIX make, the stuff that pretty much all makes understand. This is make on e.g. Solaris and *BSD.

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/make.html

This is also the make that the GNU autoconf/automake systems target, and their manuals are often helpful.

Honestly I just got used to the ;\ syntax for shell scripts in make, I can probably split some of these lines out more.

I cannot reproduce the PAGER=./vimpager man man hang here, not on OS X or XUbuntu. On OS X by the way I have to use PAGER=pwd/vimpager man man it has a different man. Let me know when you find out what the problem is.

rkitover commented 8 years ago

@lucc I should also mention that the shell code in the Makefile must be for original bourne shell not POSIX sh, if you need one to test you can use heirloom-sh from here:

http://heirloom.sourceforge.net/sh.html

lucc commented 8 years ago

Out of curiosity: Which system still uses this? I did only seriously run Linux and OS X and they always had bash (I did never use some tiny/embedded Linux distro).

This could be nr 8: Which operating systems are supported?

rkitover commented 8 years ago

Solaris uses original bourne for /bin/sh and its make uses it to run shell commands. I think it's overridable with SHELL, but there is no way to do this conditionally in UNIX make, that I know of. See the GNU autoconf portable shell guidelines, that's probably the best info on this.

lucc commented 8 years ago

Do you mean these? I just had a look at them and now I have this question:

  1. Why are we wrapping awk and sed in functions? Is there a chance that they are not available on a system or are we looking for a particular "good" version? What features do we need in these programs that are not available in all versions? Should we assume that they are POSIX compatible like the shell we are looking for?
  2. POSIX requires the -n option for head and tail. Why do we need to wrap them?
rkitover commented 8 years ago

@lucc

  1. Yes, the point is to use the best version of awk and sed available. All awk and sed code is written for original awk and original sed in any case. No you cannot assume anything is POSIX compatible.
  2. POSIX may require it but Solaris does not have this, and I don't know what other systems don't either, and why exactly is this an issue anyway? I only wrote those functions because it seemed important to you to use that syntax when available, but all versions of head and tail support -<N> and always will.
lucc commented 8 years ago

About head and tail: I think that wasn't me but @eworm-de, see #74. Am I wrong?

rkitover commented 8 years ago

@lucc yes I totally confused you with this eworm-de guy for some reason, sorry about that, my memory is fuzzy.

In which case, can we please stop putting braces around shell variables? I agreed to it because he asked me to, but it seems overkill, most people just write "$foo" and not "${foo}" in shell code.

lucc commented 8 years ago

Yes we can. (Or should I say "Yes I can" since that is something I could push to master rather quickly right now :)