[
https://issues.apache.org/jira/browse/TINKERPOP-3055?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17946901#comment-17946901
]
ASF GitHub Bot commented on TINKERPOP-3055:
-------------------------------------------
andreachild commented on code in PR #3099:
URL: https://github.com/apache/tinkerpop/pull/3099#discussion_r2057209424
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/HaltedTraverserStrategy.java:
##########
@@ -75,10 +77,10 @@ public static HaltedTraverserStrategy create(final
Configuration configuration)
@Override
public Configuration getConfiguration() {
- final Map<String, Object> map = new HashMap<>();
- map.put(STRATEGY, HaltedTraverserStrategy.class.getCanonicalName());
- map.put(HALTED_TRAVERSER_FACTORY,
this.haltedTraverserFactory.getCanonicalName());
- return new MapConfiguration(map);
+ final BaseConfiguration conf = new BaseConfiguration();
+
conf.setListDelimiterHandler(GremlinDisabledListDelimiterHandler.instance());
+ conf.setProperty(HALTED_TRAVERSER_FACTORY,
this.haltedTraverserFactory.getCanonicalName());
Review Comment:
Nit: it's not a problem since both factory types are not inner classes but
it would be best to use `this.haltedTraverserFactory.getName()` instead of
`getCanonicalName()` as the canonical name is the human readable class name
while the name is the JVM representation of the class name, which can differ
for inner classes. So if the factory classes are moved to inner classes or
another factory class is created which is an inner class, Class.forName() will
not work with the canonical name.
> withoutStrategies() mechanism in programming languages for providers
> --------------------------------------------------------------------
>
> Key: TINKERPOP-3055
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3055
> Project: TinkerPop
> Issue Type: Improvement
> Components: dotnet, go, javascript, process, python
> Affects Versions: 3.7.1
> Reporter: Stephen Mallette
> Priority: Major
>
> {{withoutStrategies()}} is in the grammar for TINKERPOP-2862. That change did
> not address its accessibility for provider strategies in language variants
> very well. As the syntax requires a {{Class}} (and for the grammar, a
> registered strategy class) you may not have that reference in a language
> variant. Users could create dummy classes as the grammar works on simple
> name, but that's not especially nice. Otoh, most users shouldn't be tinkering
> with strategies so perhaps that's ok? It could be inconvenient for notebook
> users and similar tools though to create the dummy. A simple alternative
> could just be a {{withoutStrategies(String...)}} but that's not particularly
> nice. Other ideas?
> needs a general look at all strategy construction across all languages:
> 1. check if the strategy construction makes sense in terms of types and
> syntax in each language
> 2. watch out for wrong types being parsed into {{Configuration}} which can
> lead to weird looking errors.
> 3. are there adequate tests to validate all our syntax is working. we
> technically need to test every strategy configuration options as those
> corners are where bugs can hide.
> 4. double check {{List}} vs {{Set}} syntax because {{Set}} might be preferred
> but a lot of folks will reach for {{[ ]}} just out of habit....do we want
> them failing for that? can we ease the type there without losing {{Set}} in
> type safe languages?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
