Command refactoring and more #2397

bwitham · 2018-05-21T17:52:45Z

collapse multiple commands into a single command; makes commands easier to use and reduces redundant code in some cases:
- convert commands
- conflate commands (disable cumulative conflate for now, as its not used)
- calc tiles commands
- perty commands
add stats for railways

…mand

dgisbot · 2018-05-29T18:51:15Z

hoot-rnd/src/main/cpp/hoot/rnd/cmd/PertyCmd.cpp

+    {
+      boost::shared_ptr<const MatchComparator> matchComparator =
+        PertyMatchScorer().scoreMatches(args[0], args[1]);
+      cout << MapMatchScoringUtils::getMatchScoringString(matchComparator);


Replace this usage of "::std::cout" by a logger.

Maybe some other time. Not logging this as a warning b/c that's strange and logging it as info requires updating a lot of regression tests again.

bwitham · 2018-05-29T19:13:50Z

@mattjdnv @mschicker A lot of file changes, but this was mostly focused on simplifying the various convert commands into a single command to make it easier to use. There are a few extra things listed in the description that I did along the way. Also, #2416 is a follow on issue that would clean up the convert command even further...decided not to tackle it this time around as it might be fairly involved. thanks.

dgisbot · 2018-05-29T19:26:57Z

hoot-core/src/main/cpp/hoot/core/io/DataConverter.cpp

+    {
+      if (Log::getInstance().getLevel() == Log::Info)
+      {
+        std::cout << ".";


Replace this usage of "::std::cout" by a logger.

pre-existing code from Ogr2OsmCmd

dgisbot · 2018-05-29T19:26:57Z

hoot-core/src/main/cpp/hoot/core/io/DataConverter.cpp

+      if (Log::getInstance().getLevel() == Log::Info)
+      {
+        std::cout << ".";
+        std::cout.flush();


Replace this usage of "::std::cout" by a logger.

pre-existing code from Ogr2OsmCmd

dgisbot · 2018-05-29T19:26:58Z

hoot-core/src/main/cpp/hoot/core/io/DataConverter.cpp

+
+  OsmMapReaderFactory readerFactory = OsmMapReaderFactory::getInstance();
+  if (readerFactory.hasElementInputStream(input) &&
+      //TODO: this ops restriction needs to be removed and the ops applied during streaming.


Complete the task associated to this "TODO" comment.

Don't tell me what TODO.

will be handled in #2416

dgisbot · 2018-05-29T19:27:01Z

hoot-core/src/main/cpp/hoot/core/io/DataConverter.cpp

+  }
+}
+
+void DataConverter::_convertFromOgr(const QStringList inputs, const QString output)


Refactor this function to reduce its Cognitive Complexity from 50 to the 25 allowed.

I'm sorry you're not smart enough to understand this function.

Witham: 1
Dgisbot: 0

Have to keep that guy in line :-)

I'm learning about cognitive complexity now. They list it as critical and then go on to give example open source projects that have over a thousand instances of it....so, is it really that critical?

pre-existing method from Ogr2OsmCmd; maybe can look into refactoring it into multiple less complex methods at a later date

dgisbot · 2018-05-29T21:03:44Z

hoot-rnd/src/main/cpp/hoot/rnd/cmd/PertyCmd.cpp

@@ -52,21 +57,89 @@ class PertyCmd : public BaseCommand

  virtual int runSimple(QStringList args)
  {
-    if (args.size() != 2)
+    LOG_VARD(args.size());
+    if (args.size() < 2 || args.size() > 3)
    {
      cout << getHelp() << endl << endl;


Replace this usage of "::std::cout" by a logger.

dgisbot · 2018-05-29T21:03:47Z

SonarQube analysis reported 13 issues

1 critical
11 major
1 info

Watch the comments in this conversation to review them.

7 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

EdgeString.cpp#L65: Remove this block of commented out code.
EdgeString.cpp#L66: Remove this block of commented out code.
EdgeString.cpp#L68: Remove this block of commented out code.
EdgeString.cpp#L69: Remove this block of commented out code.
EdgeString.cpp#L70: Remove this block of commented out code.
EdgeString.cpp#L71: Remove this block of commented out code.
EdgeString.cpp#L72: Remove this block of commented out code.

bmarchant · 2018-05-29T21:16:04Z

@dgisbot has a mind of its own!

bwitham · 2018-05-29T21:17:53Z

That's harsh. Silenced by dgisbot...my voice will be heard!

mattjdnv · 2018-05-30T14:52:07Z

The struggle is real.
First it was Cucumber, now sonarqube.

bmarchant · 2018-05-30T15:44:27Z

I wonder if we can get SonarQube to only report the actual error findings and not just the code smells. I like the idea of cleaning up some relevant code smells but it isn't necessary to pass the test in my opinion.

mschicker

Nice work! Approved! I agree that most of the SonarQube comments, while insightful, should not be addressed at this time.

bwitham · 2018-05-30T16:36:49Z

Yeah, I think the best setup would be to have qube report everything in PR's but have the PR only fail on errors and not smells, if that's possible. Then optionally fix smells in the same PR or do it in a later one for the more critical items. We might have to do some adjusting on the priorities on the smells. For instance, qube was calling a large complex method that needed refactoring a critical problem...I would that give that type of thing a lower priority than critical.

bwitham added 8 commits May 17, 2018 14:14

consolidate convert commands (old ones still remain for now)

1deba7f

some bug fixes

b133ae3

some more bug fixes

cf4f6a8

fix handling of file geodb dirs

2429ad5

fix a test

ebbc74f

fix bug with handling of convert ops

913fa08

fix bug with handling of PG OGR urls

a51d06b

fix invalid input specified in a test

cf94f0c

bwitham added the Category: Core label May 21, 2018

bwitham added this to the Hootenanny 2018-05-23 milestone May 21, 2018

bwitham self-assigned this May 21, 2018

bwitham added 19 commits May 21, 2018 15:00

move data conversion into its own class separate from the convert com…

de63769

…mand

collapse tile calculate commands into a single command

5a1dcdc

copyright header updates

9c17e38

collapse conflation commands into single; disable cum conflate

13ec1c1

archive some documentation

7e250c7

resolve merge conflicts

f3b03b0

resolve merge conflicts

47dd97d

fix docs

6051501

fix test

7f68e43

convert command batch processing prototype

2404f9b

remove batch processing feature

e873fad

remove batch processing feature

79d8814

remove batch processing feature

2b02237

add DataConverterTest

145c1da

add stats for railways

4e90aff

Merge branch 'develop' into 2379

a91b2fc

Merge branch '2379' into add-railway-stats

bb0a35d

fix tests

5123398

fix tests; copyright header updates

8635cff

bmarchant and others added 3 commits May 29, 2018 18:04

Removing more false assertions and protecting empty string references (…

ea20ed1

…#2415)

copyright header update

182e760

remove unneeded cache setup from convert cmd

51769ac

dgisbot reviewed May 29, 2018

View reviewed changes

bwitham requested review from mattjdnv and mschicker May 29, 2018 19:11

dgisbot reviewed May 29, 2018

View reviewed changes

some qube changes

30eae3c

dgisbot reviewed May 29, 2018

View reviewed changes

ngageoint deleted a comment from bwitham May 29, 2018

ngageoint deleted a comment from bmarchant May 29, 2018

ngageoint deleted a comment from bwitham May 29, 2018

mattjdnv approved these changes May 30, 2018

View reviewed changes

mschicker approved these changes May 30, 2018

View reviewed changes

bwitham merged commit 079d918 into develop May 30, 2018

bwitham deleted the 2379 branch May 30, 2018 16:38

bwitham restored the 2379 branch May 30, 2018 20:23

bwitham deleted the 2379 branch May 30, 2018 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command refactoring and more #2397

Command refactoring and more #2397

bwitham commented May 21, 2018 •

edited

dgisbot May 29, 2018

bwitham May 29, 2018

bwitham commented May 29, 2018 •

edited

dgisbot May 29, 2018

bwitham May 30, 2018

dgisbot May 29, 2018

bwitham May 30, 2018

dgisbot May 29, 2018

bwitham May 29, 2018 •

edited

bwitham May 30, 2018

dgisbot May 29, 2018

bwitham May 29, 2018

mattjdnv May 29, 2018

bwitham May 29, 2018

bwitham May 29, 2018

bwitham May 30, 2018

dgisbot May 29, 2018

dgisbot commented May 29, 2018

bmarchant commented May 29, 2018

bwitham commented May 29, 2018

mattjdnv commented May 30, 2018

bmarchant commented May 30, 2018

mschicker left a comment

bwitham commented May 30, 2018

Command refactoring and more #2397

Command refactoring and more #2397

Conversation

bwitham commented May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwitham commented May 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwitham May 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgisbot commented May 29, 2018

7 extra issues

bmarchant commented May 29, 2018

bwitham commented May 29, 2018

mattjdnv commented May 30, 2018

bmarchant commented May 30, 2018

mschicker left a comment

Choose a reason for hiding this comment

bwitham commented May 30, 2018

bwitham commented May 21, 2018 •

edited

bwitham commented May 29, 2018 •

edited

bwitham May 29, 2018 •

edited