You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Jezza <gi...@git.apache.org> on 2018/04/19 20:18:59 UTC
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
GitHub user Jezza opened a pull request:
https://github.com/apache/wicket/pull/276
WICKET-6550 : Unify all metadata capable objects.
Introduce a IMetadataContext that contains the current metadata
implementation.
This should allow more generic code to be written for all of the objects
currently capable of storing and handling metadata.
Ideally, this could be expanded with useful default methods.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jezza/wicket WICKET-6550
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/276.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #276
----
commit e92bffdf596bdf96d88de3bb2fe64616a390719b
Author: Jezza <je...@...>
Date: 2018-04-19T20:16:47Z
WICKET-6550 : Unify all metadata capable objects.
Introduce a IMetadataContext that contains the current metadata
implementation.
This should allow more generic code to be written for all of the objects
currently capable of storing and handling metadata.
Ideally, this could be expanded with useful default methods.
----
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182962256
--- Diff: wicket-core/src/main/java/org/apache/wicket/IMetadataContext.java ---
@@ -0,0 +1,10 @@
+package org.apache.wicket;
+
+/**
+ * @author Jezza
+ */
+public interface IMetadataContext<B, R> {
--- End diff --
I also have doubts about the interface name.
If anyone has a better suggestion, I'd be happy to swap it out.
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182965042
--- Diff: wicket-core/src/main/java/org/apache/wicket/IMetadataContext.java ---
@@ -0,0 +1,10 @@
+package org.apache.wicket;
+
+/**
+ * @author Jezza
+ */
+public interface IMetadataContext<B, R> {
--- End diff --
Javadoc would be nice!
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182876140
--- Diff: wicket-core/src/main/java/org/apache/wicket/IMetadataContext.java ---
@@ -0,0 +1,10 @@
+package org.apache.wicket;
+
+/**
+ * @author Jezza
+ */
+public interface IMetadataContext<B, R> {
--- End diff --
I don't like the names of the generics, nor the R parameter.
It was only added because the current implementation returns the object the method is enclosed in, so right now, it just conveys that type information.
Ideally, I would want to remove it.
One solution would be just to return the IMetadataContext<B> itself, but you wouldn't be able to do much with the return result, short of just setting more metadata.
---
[GitHub] wicket issue #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/276
I just realised I should probably do `IMetadataContext<B, R extends IMetadataContext<B, R>>`
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182967962
--- Diff: wicket-core/src/main/java/org/apache/wicket/Session.java ---
@@ -422,6 +422,7 @@ public Locale getLocale()
* @return The metadata
* @see MetaDataKey
*/
+ @Override
public synchronized final <M extends Serializable> M getMetaData(final MetaDataKey<M> key)
--- End diff --
There's no practical difference with implementation, but I had to move it because it was clashing with the signature from the interface.
---
[GitHub] wicket issue #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/276
Added some javadoc with some barebones explanation.
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182965271
--- Diff: wicket-core/src/main/java/org/apache/wicket/Session.java ---
@@ -422,6 +422,7 @@ public Locale getLocale()
* @return The metadata
* @see MetaDataKey
*/
+ @Override
public synchronized final <M extends Serializable> M getMetaData(final MetaDataKey<M> key)
--- End diff --
Why in Application you moved `synchronized` in the method body and here it stays as before ? Is there any difference between the implementations ?
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182876774
--- Diff: wicket-core/src/main/java/org/apache/wicket/Application.java ---
@@ -518,9 +519,13 @@ public void logResponseTarget(final IRequestHandler requestTarget)
* @throws IllegalArgumentException
* @see MetaDataKey
*/
- public final synchronized <T> Application setMetaData(final MetaDataKey<T> key, final Object object)
+ @Override
+ public final <T> Application setMetaData(final MetaDataKey<T> key, final T object)
--- End diff --
I had to change the signature of this method.
Ideally, it should have been that way anyway.
Everything still compiles, which means we weren't putting values in keys that weren't expecting the value.
I fear that some users might be doing exactly that though, so this whole IMetadataContext might have to be put off for a bigger version.
---
[GitHub] wicket issue #276: WICKET-6550 : Unify all metadata capable objects.
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on the issue:
https://github.com/apache/wicket/pull/276
Cool!
Please just add Javadoc about the `tparam`s of IMetadataContext!
LGTM!
---
[GitHub] wicket issue #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/276
I went ahead and changed it.
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/wicket/pull/276
---
[GitHub] wicket pull request #276: WICKET-6550 : Unify all metadata capable objects.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on a diff in the pull request:
https://github.com/apache/wicket/pull/276#discussion_r182968690
--- Diff: wicket-core/src/main/java/org/apache/wicket/Session.java ---
@@ -422,6 +422,7 @@ public Locale getLocale()
* @return The metadata
* @see MetaDataKey
*/
+ @Override
public synchronized final <M extends Serializable> M getMetaData(final MetaDataKey<M> key)
--- End diff --
Ah, got it.
Yeah, stupid mistake.
I think initially I mistook the error for the sync modifier, but it was the `T object` parameter.
Reverted that weird change.
---