You might even consider doing #4 first - it's OK to have a file that is only used by one other file, and that way would reduce the burden to anyone who needs to make a change to a utility file, so they don't have to make that change in multiple places.
On Thu, Nov 16, 2017 at 7:47 AM, Jin Chul Kim <jinc...@gmail.com> wrote: > Sure. Here is a plan to do this carefully. > > 1. The first change is to move only string part to test-string-expr.cc, > which just move/copy a chunk of code to a new file. There is a redundant > code in the both files between test-expr.cc and test-string-expr.cc because > we hope minimal code change. > 2. Iterate #1 approach for the remaining parts. There are several changes. > 3. Splitting test-expr.cc is finished. > 4. We can find rooms to refactor the new files because there are > redundant/unnecessary code. Several changes are required for refactoring by > an incremental manner. > > Best regards, > Jinchul > > 2017-11-16 23:51 GMT+09:00 Jim Apple <jbap...@cloudera.com>: > >> I like this idea. >> >> One thing to be careful of is to not modify the tests themselves when >> you move them. It's hard to see such changes in gerrit and it's hard >> to track them down in git history. >> >> On Thu, Nov 16, 2017 at 5:34 AM, Jin Chul Kim <jinc...@gmail.com> wrote: >> > Hi, >> > >> > https://issues.apache.org/jira/browse/IMPALA-5078 >> > >> > I'd like to get your advice if I will refactor expr-test.cc. Henry >> suggests >> > grouping tests by string instructions and move them to >> expr-string-test.cc. >> > I like his idea. You know filenames (e.g. cast-functions*, >> > timestamp-functions*, aggregate-functions*) of backend kernel code in >> exprs >> > have a same pattern, so I would suggest a pair of kernel code and unit >> > test. For example, >> > (cast-functions*, expr-cast-test.cc), >> > (timestamp-functions*, expr-timestamp-test.cc), >> > (aggregate-functions*, expr-aggregate-test.cc), >> > ... >> > >> > Do you have any suggestion or comment on this? Thanks. >> > >> > Best regards, >> > Jinchul >>
