Feature/uctags #2979
Feature/uctags #2979
Conversation
|
:-) looks like you had fun Chris (and this PR is one more reason for us to start moving to scala/kotlin) |
| /** | ||
| * Represents a container for tests of {@link CtagsUtil}. | ||
| */ | ||
| @ConditionalRun(CtagsInstalled.class) |
There was a problem hiding this comment.
CtagsInstalled is basically RuntimeEnvironment.getInstance().validateUniversalCtags() which relies on CtagsUtil functionality which is the class under test. This sounds little bit fragile to me.
There was a problem hiding this comment.
Haha I hadn't realized the convolution. Probably CtagsInstalled has outlived its usefulness, right?
|
|
||
| /** | ||
| * Creates a new instance, and attempts to configure it from the | ||
| * environment. |
There was a problem hiding this comment.
should probably explain how it differs to Ctags() constructor and when/where to use it.
|
I wonder if all these As it is currently, e.g. |
| } | ||
|
|
||
| private void initialize() { | ||
| env.validateUniversalCtags(); |
There was a problem hiding this comment.
is there a point of continuing if this returns False ?
There was a problem hiding this comment.
I am thinking if we should be more strict w.r.t. ctags validation and just terminate the indexer right in the beginning if no valid ctags implementation is found.
There was a problem hiding this comment.
Also, the work done by initialize() is repeated in every worker which seems unnecessary given all of them will share the same configuration/argv.
There was a problem hiding this comment.
Indexer is already strict when ctags validation fails.
I probably should have added a call to validateUniversalCtags() to opengrok-web instead of putting it into Ctags.initialize(), since my intention anyway was to ensure Ctags operates for --webappCtags on the same as during indexing.
There was a problem hiding this comment.
Regarding the repetition of initialize(), we already minimize Ctags construction with CtagsObjectFactory, so a small bit of redundant initialization seemed harmless.
| } | ||
| String arg = argv.get(i); | ||
| if (arg == null) { | ||
| result.append("UNDEFINED"); |
There was a problem hiding this comment.
what will this do ? and how could it happen actually ?
There was a problem hiding this comment.
I wrote that when Ctags initially had a setBinary() method and started with null binary. I retired that method in a subsequent commit but didn't remove the "UNDEFINED" handling.
|
sorry for too quick merge - @idodeclare will you be able to address @vladak comments in a new PR? |
|
Most of my comments relate to pre-existing stuff so should be part of distinct PR anyway. |
I like the idea of removing the suffix duplication, but I'm not sure how strong is the case to relocate the For a long time I thought it would be necessary to do such radical refactoring of |
Hello,
Please consider for integration this patch to accommodate the recent Universal Ctags strictness of language definitions by running an additional
ctags --list-languagesduring the verification stage and only declaring--langdefif required by the Ctags version.I also switched away from the newly deprecated syntax
--LANG-kindsto--kinds-LANG.To aid initial debugging, I updated
Indexer --helpto show the Ctags command-line when an extra level of detail is requested. This changes--help --detailedto instead show more details when-h,--helpis specified an increasing number of times. I'm open to suggestions if this seems clunky.Thank you.