[GitHub] [jena] afs commented on a change in pull request #872: JENA-2004 Implement functionality currently provided by Txn class directly in Transactional. Deprecate Txn class.

[GitBox <gi...@apache.org>]

afs commented on a change in pull request #872:
URL: https://github.com/apache/jena/pull/872#discussion_r531587137



##########
File path: jena-arq/src/main/java/org/apache/jena/riot/resultset/rw/ReadAnything.java
##########
@@ -75,7 +74,7 @@ public static SPARQLResult read(String url, Context context) {
             };
             
             if ( ds.supportsTransactions() ) 
-                return Txn.calculateWrite(ds, r);
+                return ds.calculateWrite(r);

Review comment:
       If we go with keeping `Txn`, I prefer to not make these style changes.

##########
File path: jena-arq/src/main/java/org/apache/jena/sparql/core/Transactional.java
##########
@@ -31,32 +33,12 @@
  * <pre> begin(WRITE) ... abort() or commit()</pre>
  * <p>{@code end()} is optional but preferred.
  * <p>
- * Helper code is available {@link Txn} so, for example:
- * <pre>Txn.executeRead(dataset, {@literal ()->} { ... sparql query ... });</pre>
- * <pre>Txn.executeWrite(dataset, {@literal ()->} { ... sparql update ... });</pre>
- * or use one of <tt>Txn.calculateRead</tt> and <tt>Txn.executeWrite</tt>
+ * In most cases instead of calling primitive methods such as {@code begin} and {@code commit }

Review comment:
       "In most cases" begs the question of when is it appropriate to use begin-commit.
   
   There will be application already existing that correctly uses begin-try-commit-finally and works just fine and we are keeping valid.
   
   How about:
   
   "Applications can call [examples e.g. dataset.executeRead] or use the lower-level primitives [dco for begin-commit].
   
   By the way Txn will handle some nesting cases - no nested transactions but to cope with the case of an existing transaction.
   
   

##########
File path: jena-arq/src/main/java/org/apache/jena/system/Txn.java
##########
@@ -18,18 +18,22 @@
 
 package org.apache.jena.system;
 
-import java.util.function.Supplier ;
-
 import org.apache.jena.query.TxnType;
-import org.apache.jena.sparql.core.Transactional ;
+import org.apache.jena.sparql.core.Transactional;
+
+import java.util.function.Supplier;
 
-/** Application utilities for executing code in transactions.
+/**
+ * Application utilities for executing code in transactions.
+ * <p>
+ *
+ * @deprecated Use {@link Transactional } class directly instead

Review comment:
       Not deprecated.
   
   There is existing code out there and we've been encouraging Txn use so changing to deprecated like this feels rather sudden.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org