Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-25 Thread Zack Shoylev 
zack-shoylev commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

But then it is not cross-platform. I mean, it's fairly easy to put the 
exception in, but a real fix will probably be somewhat more complex (i.e. 
hex-encoding the blob name). But I do get we want to keep this a filesystem 
provider, so it makes sense to have it follow the filesystem closely. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-25 Thread Andrew Gaul 
andrewgaul commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

Windows has other restricted characters:

http://stackoverflow.com/a/31976060/2800111

We should not strive to make the filesystem blobstore work as similar on Linux 
as Windows by crippling the former.  Rather we should make the filesystem 
blobstore as closely match real blobstores as the environment allows.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-25 Thread Zack Shoylev 
Closed #1024.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#event-836239845

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-25 Thread Zack Shoylev 
merged

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#issuecomment-256195424

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-24 Thread Zack Shoylev 
zack-shoylev commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

I think it is better to treat both \ and / as path separators everywhere. This 
way people won't be getting unexpected results and they also won't have to 
write different code for Windows and Linux, which is what they would have to do 
if we treat \ differently. Keeps our code simpler, too. Thoughts, 
counterexamples?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-24 Thread Andrew Gaul 
andrewgaul commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

Ideally we would allow `\` for Linux systems since blobstores allow this 
character.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-24 Thread Zack Shoylev 
@zack-shoylev pushed 1 commit.

0347f57  review fixes


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1024/files/597fdfb78757fccb71bc4744146d5544bcaab3c6..0347f57bad28bbc2b9567c08a77ee62b213d9c17

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Zack Shoylev 
zack-shoylev commented on this pull request.



> }
 
-   private static String denormalize(String pathToDenormalize) {
-  if (null != pathToDenormalize && pathToDenormalize.contains("/")) {
- if (BACK_SLASH.equals(File.separator)) {
-  return pathToDenormalize.replace("/", BACK_SLASH);
- }
+   /**
+* Convert path to jclouds standard (/)
+*/
+   private static String denormalize(String path) {
+  if (null != path) {
+ return path.replace("\\", "/");

I rather keep these rules OS-agnostic. Does this make sense?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Zack Shoylev 
zack-shoylev commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

This will also normalize the path on a Linux system, so I would rather leave it 
as is

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
Overall looks good, this fixes the source-code incompatibility for users on 
Windows.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#issuecomment-255481424

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
andrewgaul commented on this pull request.



>  beginIndex = 1;
   }
-  if (pathToBeCleaned.substring(pathToBeCleaned.length() - 
1).equals(File.separator))
+  if (pathToBeCleaned.substring(pathToBeCleaned.length() - 1).equals("/") 
||

Same as above.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#pullrequestreview-5330280

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
andrewgaul commented on this pull request.



> @@ -786,10 +782,11 @@ private String removeFileSeparatorFromBorders(String 
> pathToBeCleaned, boolean on
 
   // search for separator chars
   if (!onlyTrailing) {
- if (pathToBeCleaned.substring(0, 1).equals(File.separator))
+ if (pathToBeCleaned.substring(0, 1).equals("/") || 
pathToBeCleaned.substring(0, 1).equals("\\"))

Call `charAt` instead?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#pullrequestreview-5330256

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
andrewgaul commented on this pull request.



> @@ -173,9 +173,9 @@ public void testList_NoOptionSingleContainer() throws 
> IOException {
 checkForContainerContent(CONTAINER_NAME, null);
 
 // creates blobs in first container
-Set blobsExpected = 
TestUtils.createBlobsInContainer(CONTAINER_NAME, "bbb" + File.separator + "ccc"
-+ File.separator + "ddd" + File.separator + "1234.jpg", 
"4rrr.jpg", "rrr" + File.separator + "sss"
-+ File.separator + "788.jpg", "xdc" + File.separator + 
"wert.kpg");
+Set blobsExpected = 
TestUtils.createBlobsInContainer(CONTAINER_NAME, "bbb" + "/" + "ccc"
++ "/" + "ddd" + "/" + "1234.jpg", "4rrr.jpg", "rrr" + "/" + 
"sss"
++ "/" + "788.jpg", "xdc" + "/" + "wert.kpg");

Collapse the no-longer needed `+`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#pullrequestreview-5329932

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
andrewgaul commented on this pull request.



> }
 
-   private static String denormalize(String pathToDenormalize) {
-  if (null != pathToDenormalize && pathToDenormalize.contains("/")) {
- if (BACK_SLASH.equals(File.separator)) {
-  return pathToDenormalize.replace("/", BACK_SLASH);
- }
+   /**
+* Convert path to jclouds standard (/)
+*/
+   private static String denormalize(String path) {
+  if (null != path) {
+ return path.replace("\\", "/");

Same as above.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#pullrequestreview-5329869

Re: [jclouds/jclouds] Ensure that jclouds filesystem provider consistently uses / (#1024)

2016-10-21 Thread Andrew Gaul 
andrewgaul commented on this pull request.



>  * @return
 */
-   private static String normalize(String pathToBeNormalized) {
-  if (null != pathToBeNormalized && 
pathToBeNormalized.contains(BACK_SLASH)) {
- if (!BACK_SLASH.equals(File.separator)) {
-return pathToBeNormalized.replace(BACK_SLASH, File.separator);
- }
+   private static String normalize(String path) {
+  if (null != path) {

Make this conditional for Windows?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1024#pullrequestreview-5329838