jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Phillips <notificati...@github.com>
Subject Re: [jclouds/jclouds-karaf] Take into account the credentials file in GCE (#87)
Date Mon, 21 Nov 2016 13:56:40 GMT
demobox commented on this pull request.



>       * @return
      */
-    public static String getGoogleCredentialFromJsonFile(String jsonFile) {
-        try {
-            String fileContents = Files.toString(new File(jsonFile), Charsets.UTF_8);
+   public static String getGoogleCredentialFromJsonFile(String credentialValue) {

A minor point: the way the method name reads now, I think it's not clear that the method only
_tries_ to check if `credentialValue` is a file name, and returns the input value if not.
I would assume from the name that it would _always_ read from a file, and e.g. fail if the
file is not present.

How about something like:

```
public static String getGoogleCredentialFromJsonFileIfPath(String credentialValueOrPath) {
// or "filePathOrCredentialValue"?
  ...
}
```

Then, where it's used, I think it's a little clearer what is actually happening.

Alternatively, add a comment in places where the method is called? What do you think, @nacx?

-- 
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-karaf/pull/87#pullrequestreview-9454899
Mime
View raw message