You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "kwin (via GitHub)" <gi...@apache.org> on 2023/12/23 18:47:34 UTC
[PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
kwin opened a new pull request, #184:
URL: https://github.com/apache/maven-doxia/pull/184
Add a BufferingSinkProxyWrapper which buffers all calls on the underlying sink until it is flushed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435694330
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
+ */
+public class BufferingSinkProxyFactory implements SinkWrapperFactory {
+
+ private static final class MethodWithArguments {
+ private final Method method;
+ private final Object[] args;
+
+ MethodWithArguments(Method method, Object[] args) {
+ super();
+ this.method = method;
+ this.args = args;
+ }
+
+ void invoke(Object object) {
+ try {
+ method.invoke(object, args);
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ throw new IllegalStateException("Could not call buffered method " + method, e);
+ }
Review Comment:
What is the computational overhead by using reflection? Ignorable?
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/SinkWrapperFactoryComparator.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.util.Comparator;
+
+/** Sorts the given {@link SinkWrapperFactory}s so that the one with the highest rank comes first (i.e. order by descending ranking) */
Review Comment:
Didn't we agree on the term 'priority'?
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/PipelineSink.java:
##########
@@ -28,8 +28,10 @@
/**
* May be used to invoke the same method on a List of Sinks.
+ * Consider using the more flexible {@link SinkWrapper} approach which doesn't require the use of dynamic proxies.
Review Comment:
If you consider it better suited, I would trust your judgement here.
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
Review Comment:
proxied sink?
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
+ */
+public class BufferingSinkProxyFactory implements SinkWrapperFactory {
+
+ private static final class MethodWithArguments {
+ private final Method method;
+ private final Object[] args;
+
+ MethodWithArguments(Method method, Object[] args) {
+ super();
+ this.method = method;
+ this.args = args;
+ }
+
+ void invoke(Object object) {
+ try {
+ method.invoke(object, args);
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ throw new IllegalStateException("Could not call buffered method " + method, e);
+ }
+ }
+ }
+
+ public interface BufferingSink extends Sink {
+ // just a marker interface
+ Sink getBufferedSink();
+ }
+
+ private static final class BufferingSinkProxy implements InvocationHandler {
+ private final Queue<MethodWithArguments> bufferedInvocations;
+ private final Sink delegate;
+ private static final Method FLUSH_METHOD;
+ private static final Method GET_BUFFERED_SINK_METHOD;
+
+ static {
+ try {
+ FLUSH_METHOD = Sink.class.getMethod("flush");
+ GET_BUFFERED_SINK_METHOD = BufferingSink.class.getMethod("getBufferedSink");
+ } catch (NoSuchMethodException | SecurityException e) {
+ throw new IllegalStateException("Could not find flush method in Sink!", e);
+ }
+ }
+
+ BufferingSinkProxy(Sink delegate) {
+ bufferedInvocations = new LinkedList<>();
+ this.delegate = delegate;
+ }
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if (method.equals(FLUSH_METHOD)) {
+ bufferedInvocations.forEach(i -> i.invoke(delegate));
+ bufferedInvocations.clear();
+ } else if (method.equals(GET_BUFFERED_SINK_METHOD)) {
+ return delegate;
+ } else {
+ bufferedInvocations.add(new MethodWithArguments(method, args));
+ }
+ return null;
+ }
+ }
+
+ @Override
+ public Sink createWrapper(Sink delegate) {
+ BufferingSinkProxy proxy = new BufferingSinkProxy(delegate);
+ return (Sink) Proxy.newProxyInstance(
+ delegate.getClass().getClassLoader(), new Class<?>[] {BufferingSink.class}, proxy);
+ }
+
+ public static BufferingSink castAsBufferingSink(Sink sink) {
+ if (sink instanceof BufferingSink) {
+ return (BufferingSink) sink;
+ } else {
+ throw new IllegalArgumentException("The given sink is no BufferingSink but a " + sink.getClass());
Review Comment:
...is not a...
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/RandomAccessSink.java:
##########
@@ -27,19 +27,21 @@
import java.util.List;
import org.apache.maven.doxia.sink.Sink;
-import org.apache.maven.doxia.sink.SinkEventAttributes;
import org.apache.maven.doxia.sink.SinkFactory;
/**
* The RandomAccessSink provides the ability to create a {@link Sink} with hooks.
* A page can be prepared by first creating its structure and specifying the positions of these hooks.
* After specifying the structure, the page can be filled with content from one or more models.
* These hooks can prevent you to have to loop over the model multiple times to build the page as desired.
- *
+ * Consider using the {@link BufferingSinkProxyFactory} instead which allows to buffer on the (higher) Sink API level
Review Comment:
Same as `PipelineSink`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435720272
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/PipelineSink.java:
##########
@@ -28,8 +28,10 @@
/**
* May be used to invoke the same method on a List of Sinks.
+ * Consider using the more flexible {@link SinkWrapper} approach which doesn't require the use of dynamic proxies.
Review Comment:
Deprecated in https://github.com/apache/maven-doxia/pull/184/commits/2695cb63c7b638f5a01c961f320c5a785eb192b3.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435718876
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
+ */
+public class BufferingSinkProxyFactory implements SinkWrapperFactory {
+
+ private static final class MethodWithArguments {
+ private final Method method;
+ private final Object[] args;
+
+ MethodWithArguments(Method method, Object[] args) {
+ super();
+ this.method = method;
+ this.args = args;
+ }
+
+ void invoke(Object object) {
+ try {
+ method.invoke(object, args);
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ throw new IllegalStateException("Could not call buffered method " + method, e);
+ }
Review Comment:
According to https://stackoverflow.com/a/1856285/5155923 it should be neglectable in this case.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435720203
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
Review Comment:
Rewritten in https://github.com/apache/maven-doxia/pull/184/commits/2695cb63c7b638f5a01c961f320c5a785eb192b3
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/RandomAccessSink.java:
##########
@@ -27,19 +27,21 @@
import java.util.List;
import org.apache.maven.doxia.sink.Sink;
-import org.apache.maven.doxia.sink.SinkEventAttributes;
import org.apache.maven.doxia.sink.SinkFactory;
/**
* The RandomAccessSink provides the ability to create a {@link Sink} with hooks.
* A page can be prepared by first creating its structure and specifying the positions of these hooks.
* After specifying the structure, the page can be filled with content from one or more models.
* These hooks can prevent you to have to loop over the model multiple times to build the page as desired.
- *
+ * Consider using the {@link BufferingSinkProxyFactory} instead which allows to buffer on the (higher) Sink API level
Review Comment:
Ok, deprecated it in https://github.com/apache/maven-doxia/pull/184/commits/2695cb63c7b638f5a01c961f320c5a785eb192b3.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435685799
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/PipelineSink.java:
##########
@@ -28,8 +28,10 @@
/**
* May be used to invoke the same method on a List of Sinks.
+ * Consider using the more flexible {@link SinkWrapper} approach which doesn't require the use of dynamic proxies.
Review Comment:
Should we deprecate in favour of `SinkWrapper`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435719871
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
+ */
+public class BufferingSinkProxyFactory implements SinkWrapperFactory {
+
+ private static final class MethodWithArguments {
+ private final Method method;
+ private final Object[] args;
+
+ MethodWithArguments(Method method, Object[] args) {
+ super();
+ this.method = method;
+ this.args = args;
+ }
+
+ void invoke(Object object) {
+ try {
+ method.invoke(object, args);
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ throw new IllegalStateException("Could not call buffered method " + method, e);
+ }
Review Comment:
Right, thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435721892
##########
doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapper.java:
##########
Review Comment:
> Should we move this to another package? Maybe `o.a.m.doxia.sink.impl` is better, but this is clearly no impl details but rather class to be reused among doxia modules and potentially other consumers. Usually `impl` packages are used for packages which should not be reused outside the current JAR. So maybe `o.a.m.doxia.sink` is better, OTOH all other classes in `o.a.m.doxia.sink.impl` are probably not belonging to an impl package either, as all of them are reused. WDYT @michael-o?
I agree with you completely. Only interfaced and package private stuff belongs here. Abstract ones and reusables don't. Though, since the rest is here and we cannot move them, I'd go with the compromise and put in into `impl` to make it consistently weird -- at least.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin merged PR #184:
URL: https://github.com/apache/maven-doxia/pull/184
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435685833
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/RandomAccessSink.java:
##########
@@ -27,19 +27,21 @@
import java.util.List;
import org.apache.maven.doxia.sink.Sink;
-import org.apache.maven.doxia.sink.SinkEventAttributes;
import org.apache.maven.doxia.sink.SinkFactory;
/**
* The RandomAccessSink provides the ability to create a {@link Sink} with hooks.
* A page can be prepared by first creating its structure and specifying the positions of these hooks.
* After specifying the structure, the page can be filled with content from one or more models.
* These hooks can prevent you to have to loop over the model multiple times to build the page as desired.
- *
+ * Consider using the {@link BufferingSinkProxyFactory} instead which allows to buffer on the (higher) Sink API level
Review Comment:
Should we deprecate in favour of `SinkWrapper`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435685688
##########
doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapper.java:
##########
Review Comment:
I moved now to the existing `o.a.m.doxia.sink.impl` although I find the `impl` package name part confusing...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435695159
##########
doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapper.java:
##########
Review Comment:
I need to think about this...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435679435
##########
doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapper.java:
##########
Review Comment:
Should we move this to another package? Maybe `o.a.m.doxia.sink.impl` is better, but this is clearly no impl details but rather class to be reused among doxia modules and potentially other consumers. Usually `impl` packages are used for packages which should not be reused outside the current JAR. So maybe `o.a.m.doxia.sink` is better, OTOH all other classes in `o.a.m.doxia.sink.impl` are probably not belonging to an impl package either, as all of them are reused. WDYT @michael-o?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435679435
##########
doxia-core/src/main/java/org/apache/maven/doxia/parser/SinkWrapper.java:
##########
Review Comment:
Should we move this to another package? Maybe `o.a.m.doxia.sink.impl` is better, but this is clearly no impl details but rather class to be reused among doxia modules and potentially other consumers. Usually `impl` packages are used for packages which should not be reused outside the current JAR. So maybe `o.a.m.doxia.sink` is better, OTOH all other classes in `o.a.m.doxia.sink.impl` are probably not belonging to an impl package either, as all of them are reused. WDYT @michael-o
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DOXIA-711] Introduce SinkWrapper and according SinkWrapperFactory [maven-doxia]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #184:
URL: https://github.com/apache/maven-doxia/pull/184#discussion_r1435720047
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/BufferingSinkProxyFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.LinkedList;
+import java.util.Queue;
+
+import org.apache.maven.doxia.sink.Sink;
+
+/**
+ * Buffers all method calls on the wrapped Sink until the proxy sink's method {@link Sink#flush()} is called.
+ */
+public class BufferingSinkProxyFactory implements SinkWrapperFactory {
+
+ private static final class MethodWithArguments {
+ private final Method method;
+ private final Object[] args;
+
+ MethodWithArguments(Method method, Object[] args) {
+ super();
+ this.method = method;
+ this.args = args;
+ }
+
+ void invoke(Object object) {
+ try {
+ method.invoke(object, args);
+ } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
+ throw new IllegalStateException("Could not call buffered method " + method, e);
+ }
+ }
+ }
+
+ public interface BufferingSink extends Sink {
+ // just a marker interface
+ Sink getBufferedSink();
+ }
+
+ private static final class BufferingSinkProxy implements InvocationHandler {
+ private final Queue<MethodWithArguments> bufferedInvocations;
+ private final Sink delegate;
+ private static final Method FLUSH_METHOD;
+ private static final Method GET_BUFFERED_SINK_METHOD;
+
+ static {
+ try {
+ FLUSH_METHOD = Sink.class.getMethod("flush");
+ GET_BUFFERED_SINK_METHOD = BufferingSink.class.getMethod("getBufferedSink");
+ } catch (NoSuchMethodException | SecurityException e) {
+ throw new IllegalStateException("Could not find flush method in Sink!", e);
+ }
+ }
+
+ BufferingSinkProxy(Sink delegate) {
+ bufferedInvocations = new LinkedList<>();
+ this.delegate = delegate;
+ }
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if (method.equals(FLUSH_METHOD)) {
+ bufferedInvocations.forEach(i -> i.invoke(delegate));
+ bufferedInvocations.clear();
+ } else if (method.equals(GET_BUFFERED_SINK_METHOD)) {
+ return delegate;
+ } else {
+ bufferedInvocations.add(new MethodWithArguments(method, args));
+ }
+ return null;
+ }
+ }
+
+ @Override
+ public Sink createWrapper(Sink delegate) {
+ BufferingSinkProxy proxy = new BufferingSinkProxy(delegate);
+ return (Sink) Proxy.newProxyInstance(
+ delegate.getClass().getClassLoader(), new Class<?>[] {BufferingSink.class}, proxy);
+ }
+
+ public static BufferingSink castAsBufferingSink(Sink sink) {
+ if (sink instanceof BufferingSink) {
+ return (BufferingSink) sink;
+ } else {
+ throw new IllegalArgumentException("The given sink is no BufferingSink but a " + sink.getClass());
Review Comment:
Done
##########
doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/SinkWrapperFactoryComparator.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.doxia.sink.impl;
+
+import java.util.Comparator;
+
+/** Sorts the given {@link SinkWrapperFactory}s so that the one with the highest rank comes first (i.e. order by descending ranking) */
Review Comment:
Yes indeed, fixed now.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org