add implementation of unlet $FOO by mattn · Pull Request #2855 · vim/vim

mattn · 2018-05-01T07:23:48Z

OS compatibility
Error handling on some OS which doesn't work unsetenv.
Add tests

codecov-io · 2018-05-01T07:43:08Z

Codecov Report

Merging #2855 into master will increase coverage by 0.02%.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##           master    #2855      +/-   ##
==========================================
+ Coverage   75.53%   75.55%   +0.02%     
==========================================
  Files          92       92              
  Lines      134866   134878      +12     
==========================================
+ Hits       101867   101911      +44     
+ Misses      32999    32967      -32

Impacted Files	Coverage Δ
src/misc1.c	`84.31% <0%> (-0.06%)`	⬇️
src/eval.c	`81.35% <12.5%> (-0.13%)`	⬇️
src/ex_cmds.c	`79.38% <0%> (-0.23%)`	⬇️
src/ex_docmd.c	`75.95% <0%> (-0.02%)`	⬇️
src/if_py_both.h	`76.59% <0%> (ø)`	⬆️
src/screen.c	`76.59% <0%> (+0.04%)`	⬆️
src/os_unix.c	`54.47% <0%> (+0.04%)`	⬆️
src/gui.c	`49.2% <0%> (+0.1%)`	⬆️
src/term.c	`59.78% <0%> (+0.1%)`	⬆️
src/channel.c	`83.1% <0%> (+0.11%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 402c839...f50a4d8. Read the comment docs.

k-takata · 2018-05-01T09:41:12Z

src/misc1.c

+#ifndef _WIN32
+    unsetenv((char *)var);
+#elif defined(HAVE_UNSETENV)
+    mch_setenv((char *)var, "", 0);


Are these conditions correct?
Shouldn't be as follows?

#ifdef HAVE_UNSETENV unsetenv((char *)var); #elif defined(_WIN32) mch_setenv((char *)var, "", 0);

ZyX-I · 2018-05-01T13:27:46Z

src/testdir/test_unlet_env.vim

+  endfor
+  call assert_equal(0, found)
+
+  unlet $MUST_NOT_BE_AN_ERROR


You should probably also check for unexistence of this variable first. It is also not obvious from the variable name that you check for unletting unexistent variable. If writing tests for Neovim I would check that with eq(0, exc_exec('unlet $XXX_NONEXSTENT_VAR_XXX')): variable name states that it is unexistent and surrounding code states that it is checking for no exceptions.

Though why not just use unlet $FOOBAR again?

I'm thinking non-existence variable should not be an error since :echo $NOT_FOUND_ENV doesn't make an error.

bash doesn't make it an error.

$ unset FOOOOOOOOOOOOOO

@mattn I am not saying it should be an error. I am saying that variable may exist and it is good to check this is not the case first, primary to make it obvious what exactly test is checking. And/or name variable so that it is obvious that it is not supposed to exist. Or use $FOOBAR as it was just deleted.

Though bash unset FOOOO is not relevant. unlet g:xxx_nonexistent_var_xxx will throw, unlet! … won’t and this is VimL and not bash.

As long as non-existence variable doesn't make an error with reference, I'm thinking unlet should not make error. @brammool How do you think?

ZyX-I · 2018-05-01T13:28:45Z

src/testdir/test_unlet_env.vim

+" Test for unlet $FOOBAR.
+
+func Test_UnletEnv()
+  let envcmd = has('win32') ? 'set' : 'env'


And before doing that you need to check that unlet $... does not throw and skip the test if it does. Throwing in some environment is expected behaviour.

ZyX-I · 2018-05-01T13:30:56Z

src/testdir/test_unlet_env.vim

@@ -0,0 +1,25 @@
+" Test for unlet $FOOBAR.


New file needs to be listed in a number of places. Find some commits where Bram adds new test file, I do not know the details.

src/Makefile and src/testdir/Make_all.mak, or src/testdir/test_alot.vim should be updated.

ZyX-I · 2018-05-01T13:34:36Z

src/misc1.c

+#elif defined(HAVE_UNSETENV)
+    unsetenv((char *)var);
+#else
+    EMSG(_("E319: Sorry, the command is not available in this version"));


Do not copy message text over, if needed place that in globals.h near other error messages and use in two places. Though I would be against this message: command is available, specific part of command’s functionality is missing and it deserves its own message.

k-takata · 2018-05-01T15:58:36Z

src/Makefile

 	test_windows_home \
 	test_wordcount \
 	test_writefile \
+	test_unlet_env \


This should be in the alphabetical order.

brammool · 2018-05-02T20:43:11Z

mattn commented on this pull request. > + if kv == 'FOOBAR=test' + let found = 1 + endif + endfor + call assert_equal(1, found) + + unlet $FOOBAR + let found = 0 + for kv in split(system(envcmd), "\r*\n") + if kv == 'FOOBAR=test' + let found = 1 + endif + endfor + call assert_equal(0, found) + + unlet $MUST_NOT_BE_AN_ERROR As long as non-existence variable doesn't make an error with reference, I'm thinking unlet should not make error. @brammool How do you think?

AFAIK most programs don't complain when trying to remove an environment variable that doesn't exist. The functionality will be mainly to make sure the environment variable does not exist, thus ignoring errors seems right. In the few cases it should actually be an error, one can first check that the variable does exist. On the other hand, you can always use ":unlet! $MUST_NOT_BE_AN_ERROR". That's consistent with unletting variables. Having to explain the exception isn't nice. unlet MUST_NOT_BE_AN_ERROR " error unlet $MUST_NOT_BE_AN_ERROR " no error That's unexpected.

…

-- I still remember when I gave up Smoking, Drinking and Sex. It was the most *horrifying* hour of my life! /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

mattn · 2018-05-05T13:58:44Z

Fixed some pointed in this reviews.

k-takata · 2018-05-07T04:33:07Z

Many unrelated changes are shown in the diff. Maybe it's better to rebase on top of the master branch.
Could you also update the document?

closes vim#1116

k-takata · 2018-05-07T04:43:49Z

runtime/doc/eval.txt

 			variables are automatically deleted when the function
 			ends.

+:unl[et] ${env-name}				*:let-environment* *:let-$*


s/let/unlet/g

k-takata · 2018-05-07T04:44:23Z

runtime/doc/eval.txt

 			ends.

+:unl[et] ${env-name}				*:let-environment* *:let-$*
+			Rermove environment variable {env-name}.


s/Rermove/Remove/

mattn · 2018-05-07T05:19:42Z

@k-takata Thank your pointing this. Probably I was sleepy.

k-takata · 2018-05-07T05:25:08Z

runtime/doc/eval.txt


-:unl[et] ${env-name}				*:let-environment* *:let-$*
-			Rermove environment variable {env-name}.
+:unl[et] ${env-name}


Oh, did you totally remove the tags?
I thought they should be: *:unlet-environment* *:unlet-$*.

Problem: Cannot use :unlet for an environment variable. Solution: Make it work. Use unsetenv() if available. (Ken Takata, closes vim#2855)

Problem: Cannot use :unlet for an environment variable. Solution: Make it work. Use unsetenv() if available. (Ken Takata, closes vim/vim#2855) vim/vim@137374f

Problem: Cannot use :unlet for an environment variable. Solution: Make it work. Use unsetenv() if available. (Yasuhiro Matsumoto, closes vim/vim#2855) vim/vim@137374f

k-takata reviewed May 1, 2018

View reviewed changes

ZyX-I reviewed May 1, 2018

View reviewed changes

k-takata reviewed May 1, 2018

View reviewed changes

mattn mentioned this pull request May 5, 2018

add environ() #2875

Closed

mattn changed the title ~~[WIP] add implementation of unlet $FOO~~ add implementation of unlet $FOO May 5, 2018

mattn added 4 commits May 7, 2018 13:35

add implementation of unlet $FOO

4db94e5

closes vim#1116

add test

6ad4521

check HAVE_UNSETENV

b9dfa6b

fix condition

c507893

mattn force-pushed the unsetenv branch from c12e01b to c507893 Compare May 7, 2018 04:36

update doc

55f6fef

k-takata reviewed May 7, 2018

View reviewed changes

fix doc

d510f1c

mattn force-pushed the unsetenv branch from d86db3d to d510f1c Compare May 7, 2018 05:06

k-takata reviewed May 7, 2018

View reviewed changes

fix doc

f50a4d8

brammool closed this in 137374f May 13, 2018

janlazo mentioned this pull request Oct 2, 2018

[RDY] vim-patch:8.0.1832,8.1.0047 neovim/neovim#9078

Merged

Uh oh!

Conversation

mattn commented May 1, 2018

Uh oh!

codecov-io commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZyX-I May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brammool commented May 2, 2018 via email

Uh oh!

mattn commented May 5, 2018

Uh oh!

k-takata commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattn commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented May 1, 2018 •

edited

Loading

ZyX-I May 1, 2018 •

edited

Loading