Fujii Masao
masao****@gmail*****
2015年 8月 28日 (金) 14:14:48 JST
On Mon, Aug 24, 2015 at 3:09 PM, <ishii****@nttda*****> wrote: > Thank you for review. > I updated the patch. > >> > Changes from the last time: >> > ・Delete unnecessary "CREATE USER" >> > (We use "CREATE USER" only when we create normal user.) >> >> You can use SET ROLE and RESET ROLE command in order to do permission >> test, instead of adding textporter-close test. > > I fixed to use SET ROLE instead of textporter-close test. > >> > expected/ludia_funcs.out:789: trailing whitespace. >> > +\c euc_db >> > expected/ludia_funcs.out:795: trailing whitespace. >> > +\c contrib_regression >> > sql/ludia_funcs.sql:176: trailing whitespace. >> > +\c euc_db >> > sql/ludia_funcs.sql:180: trailing whitespace. >> > +\c contrib_regression >> >> There are some unnecessary trailing whitespaces in >> 000_ludia_funcs_regress.patch as follows. > > I fixed to delete unnecessary trailing whitespaces. > >> > $ patch -d. -p1 < >> > ../patch/ludia_funcs_regress_patch/001_ludia_funcs_regress_debug_opt >> io >> > n.patch >> > patching file Makefile >> > Hunk #1 FAILED at 33. >> > 1 out of 1 hunk FAILED -- saving rejects to file Makefile.rej >> I could not apply 001_ludia_funcs_regress_debug_option.patch >> successfully. >> >> It looks like both file debug_option.patch and textporter.patch have >> wrong prefix number each other. >> debug_options patch should be applied after textporter patch applied. > > I fixed the number. > ・000_ludia_funcs_regress.patch > ・001_ludia_funcs_regress_textporter.patch sql/ludia_funcs.sql executes "LOAD '$libdir/ludia_funcs'" before registering ludia_funcs extension, but sql/textporter.sql does not. This makes me wonder if such LOAD command is really required. Thought? +SELECT pgs2snippet1(1,300,1,'∇','∇',0,'データベース',pgs2textporter1('/tmp/sample.txt')); Could you tell me who will move sample.txt to /tmp? You are thinking that each developer will need to move that to /tmp manually whenever he or she wants to run the regression test for textporter? ISTM that it's not good approach. In pg_bigm, this kind of input file is placed in "data" directory and each pg_bigm regression test tries to read it from "data" directory. Like pg_bigm, what about placing the input file in "data" directory? Please see the regression test in pg_bigm to understand the idea. +SELECT pgs2snippet1(1,300,1,'∇','∇',0,'データベース',pgs2textporter1('/tmp/sample.txt')); +SELECT pgs2snippet1(1,300,1,'∇','∇',0,'aaa',pgs2textporter1('/tmp/sample.txt')); + +SELECT pgs2snippet1(1,300,1,'∇','∇',0,repeat('x',300),repeat('x',300)); +SELECT pgs2snippet1(1,300,1,'∇','∇',0,repeat('x',300),pgs2textporter1('/tmp/sample.txt')); Could you tell me why these four tests are required? ISTM that the first test is enough. I could not understand the purposes of other three tests. Could you elaborate them? +SELECT pgs2textporter1('/tmp/test.docx'); Could you include test.docx file in the patch? This file also needs to be placed in "data" directory, I think. +SELECT pgs2textporter1('/tmp/test.xlsx'); Same as above. +SELECT pgs2textporter1('/tmp/test.docx') FROM generate_series(1,10); Could you tell me the purpose of this test? +SELECT pgs2textporter1(); +SELECT pgs2textporter1('/tmp/test.docx', 1); I think that it's not worth doing these tests. +CREATE DATABASE euc_db ENCODING 'EUC_JP' TEMPLATE template0; +\c euc_db +CREATE EXTENSION ludia_funcs; +SELECT pgs2textporter1('/tmp/test.docx'); You should change this test according to the existing regression test that I recently committed. +SET ROLE 'testuser'; +SET ludia_funcs.textporter_error = 'notice'; +RESET ROLE; I think that it's not worth doing this test. Regards, -- Fujii Masao
Fork