Polish #1938

izeye · 2018-08-22T15:41:09Z

This PR fixes some typos and polishes trivial stuff.

snicoll

Thanks for the PR @izeye! I've added a few suggestions to better explain the rationale. Does that make sense?

snicoll · 2018-08-22T15:50:14Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCache.java

@@ -28,7 +28,7 @@

 /**
 * {@link org.springframework.cache.Cache} implementation on top of a
- * {@link javax.cache.Cache} instance.
+ * {@link Cache} instance.


That full package name is actually on purpose as the Spring Cache abstraction has also a concept of Cache.

@snicoll If so, it should be {@link Cache javax.cache.Cache}, not {@link javax.cache.Cache} as the latter will be rendered as just Cache in Javadoc. Is it okay to change to {@link Cache javax.cache.Cache}?

See https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/cache/jcache/JCacheCache.html

Oh dear, good catch. Yes please.

snicoll · 2018-08-22T15:50:26Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCacheManager.java

@@ -28,7 +28,7 @@

 /**
 * {@link org.springframework.cache.CacheManager} implementation
- * backed by a JCache {@link javax.cache.CacheManager}.
+ * backed by a JCache {@link CacheManager}.


snicoll · 2018-08-22T15:50:42Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCacheManager.java

@@ -53,22 +53,22 @@ public JCacheCacheManager() {

 	/**
 	 * Create a new JCacheCacheManager for the given backing JCache.
-	 * @param cacheManager the backing JCache {@link javax.cache.CacheManager}
+	 * @param cacheManager the backing JCache {@link CacheManager}


snicoll · 2018-08-22T15:50:50Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCacheManager.java

 	 */
 	public JCacheCacheManager(CacheManager cacheManager) {
 		this.cacheManager = cacheManager;
 	}


 	/**
-	 * Set the backing JCache {@link javax.cache.CacheManager}.
+	 * Set the backing JCache {@link CacheManager}.


snicoll · 2018-08-22T15:50:56Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCacheManager.java

 	 */
 	public void setCacheManager(@Nullable CacheManager cacheManager) {
 		this.cacheManager = cacheManager;
 	}

 	/**
-	 * Return the backing JCache {@link javax.cache.CacheManager}.
+	 * Return the backing JCache {@link CacheManager}.


sbrannen · 2018-08-22T15:49:21Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCache.java

@@ -28,7 +28,7 @@

 /**
 * {@link org.springframework.cache.Cache} implementation on top of a
- * {@link javax.cache.Cache} instance.


Without the fully qualified package name, we'd want to have "JCache" there in addition.

So we might as well just leave it as is for clarity, as was originally intended.

Thus, please either undo this change or prefix that with "JCache" as is done elsewhere in the Javadoc for this class.

@sbrannen It looks the same concern as @snicoll 's #1938 (comment). I answered there and let me know what you think.

sbrannen · 2018-08-22T15:50:09Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCache.java

@@ -42,15 +42,15 @@


 	/**
-	 * Create an {@link org.springframework.cache.jcache.JCacheCache} instance.
+	 * Create a {@link org.springframework.cache.jcache.JCacheCache} instance.


The package name starts with an "o". Thus this is grammatically correct.

Please undo this change.

@sbrannen "a" is intended for the rendered link in Javadoc, "JCacheCache", not "org.springframework.cache.jcache.JCacheCache". Am I missing something?

See https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/cache/jcache/JCacheCache.html

Hmmm.... good question!

I believe we (i.e., the collective core Spring Team) were thinking that the fully qualified class name (FQCN) shows up in the generated HTML when linked via FQCN.

So, if that's not the case, that would mean there is a larger issue across the entire code base in the core Spring Framework, since we routinely use "an" instead of "a" in such cases.

Perhaps it's better to address that nuanced feature of Javadoc in a dedicated JIRA issue.

@snicoll, thoughts?

@sbrannen thanks. Could you please raise a SPR issue for this? I got caught as well and I am wondering if that's not a new feature of javadoc or something (or I've been wrong this all time).

Indeed! I am also wondering if this was a change to how Javadoc works in a recent release (perhaps in JDK 8?).

And well... if you've been wrong this whole time, so have I and many others. 😉

I'll raise a JIRA issue to track this.

Done: https://jira.spring.io/browse/SPR-17208

sbrannen · 2018-08-22T15:50:18Z

spring-context-support/src/main/java/org/springframework/cache/jcache/JCacheCache.java

 	 * @param jcache backing JCache Cache instance
 	 */
 	public JCacheCache(Cache<Object, Object> jcache) {
 		this(jcache, true);
 	}

 	/**
-	 * Create an {@link org.springframework.cache.jcache.JCacheCache} instance.
+	 * Create a {@link org.springframework.cache.jcache.JCacheCache} instance.


The package name starts with an "o". Thus this is grammatically correct.

Please undo this change.

@snicoll

@snicoll had reviewed this PR simultaneously.

sbrannen · 2018-08-22T16:02:41Z

@snicoll, after our combined reviews, it appears that the only remaining polishing would be the removal of the extra space in the exception message.

Thus, I've dismissed my review and will let you take it from here.

Cheers,

Sam

izeye · 2018-08-22T17:27:49Z

@snicoll @sbrannen Thanks for the feedback! I've updated this PR as suggested.

* pr/1938: Polish

snicoll · 2018-08-23T07:49:50Z

Thanks a lot @izeye, this is now merged in master.

snicoll reviewed Aug 22, 2018

View reviewed changes

snicoll self-assigned this Aug 22, 2018

sbrannen previously requested changes Aug 22, 2018

View reviewed changes

snicoll added documentation labels Aug 22, 2018

This comment has been minimized.

Sign in to view

Polish

e56652a

izeye force-pushed the polish-20180823 branch from 38a00cc to e56652a Compare August 22, 2018 17:26

snicoll approved these changes Aug 23, 2018

View reviewed changes

snicoll removed the waiting-for-feedback label Aug 23, 2018

snicoll closed this in 4db5d28 Aug 23, 2018

snicoll added a commit that referenced this pull request Aug 23, 2018

Merge pull request #1938 from izeye:polish-20180823

7fdd137

* pr/1938: Polish

izeye deleted the polish-20180823 branch August 23, 2018 09:09

spring-projects-issues mentioned this pull request Jan 11, 2019

Fix grammar in JavaDoc for fully qualified links [SPR-17208] #21741

Closed

Polish #1938

Polish #1938

Uh oh!

Conversation

izeye commented Aug 22, 2018

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

sbrannen commented Aug 22, 2018

Uh oh!

izeye commented Aug 22, 2018

Uh oh!

snicoll commented Aug 23, 2018

Uh oh!

Uh oh!