Use java.nio.file for file/path handling #750 #866

vivekscl · 2018-03-28T12:30:11Z

Fixes #750
Fixes #554

Things to take note of

The performance test in PersonListPanelTest is rather fickle. It fails sometimes, while passes in others. As such, TEST_TIMEOUT will be set to 12000 as a temporary measure until PersonListPanelTest: fix flaky test #863 is merged.
Even though many classes in the UI component use String arguments for file names for Class#getResource(String), the file names were changed to be of type Path and then converted to type String when needed. Not entirely sure if this is needed, but since using Strings for file names is still error prone I refactored it anyway. Do let me know if this shouldn't be the case.

Issues to raise

Many parts of the codebase have methods that declare that they throw an IOException even though it is never thrown in the first place. One example is JsonUserPrefsStorage#readUserPrefs(Path).
A few methods have return values that are never used. One such example is FileUtil#createFile(File).
Many methods can be made to have a more restrictive access modifier. One such case would be the methods in AddressBookSystemTest which could be made to be private. Not sure if this was intended for future use though.
UserPrefs#getAddressBookName is unused.

pyokagan

Request a review from me when ready.

CanIHasReview-bot · 2018-03-30T19:33:43Z

v1

@vivekscl submitted v1 for review.

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/866/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan

Refactor codebase to use java.nio.file for file/path handling

String operations are used for the construction of file paths. Also,
many APIs in the codebase take in String arguments as file paths.

This method of file/path handling is error prone.

Error prone because....

Let's refactor the codebase to use java.nio.file for file/path 
handling.

So, how does the usage of java.nio.file improve things?

Besides java.nio.file providing a platform-independent way to perform operations on paths, the usage of Path instead of String all over our code base will allow us to properly separate the concerns of parsing/stringifying paths and manipulating paths.

This can be compared to how we model our address book using domain objects (e.g. Person, AddressBook) instead of using primitive data types. Conversion between the domain objects and strings/pixels on the screen is done explicitly at specific points of the program, in the controller, view and storage layers.

Using java.io.File will also yield the same benefits, except that it has been superceded by java.nio.file.

Since Path is meant to replace File, there should probably be a commit before or after this commit to convert all usages of File in the code base to Path, especially in FileUtil, XmlUtil and JsonUtil. Yes, some conversions to File may be needed when using legacy APIs, but that should only be done at at the boundary of the legacy API itself.

pyokagan · 2018-03-31T03:06:50Z

src/main/java/seedu/address/MainApp.java

@@ -116,13 +118,13 @@ private void initLogging(Config config) {
     */
    protected Config initConfig(String configFilePath) {


Shouldn't this be taking in a Path as well?

In this method, there's a possibility that the MainApp#getApplicationParameter(String) returns a null String and causes the Paths.get(String) method to throw a NullPointerException. So I didn't add it in this case. A workaround would be to add a null check before returning the path but that would be more complicated than just returning a String right?

Well, putting aside the null check for the moment...

Would it be right for the application to crash with an InvalidPathException if the user inputs an invalid path?

At the boundaries between user input and the application you need to do a proper validation check, so yes you do need the complexity.

Oh what I meant is that, isn't there already a check where if the String is null, the default config file path will be used instead? So in this case, it's redundant to add an extra null check right?

Yes, it is redundant. But you'll need to balance it against the "handle errors properly" requirement that I mentioned up thread.

Ahh, I understand now. Will make the changes accordingly. Thanks for clearing up my doubts! :)

pyokagan · 2018-03-31T03:13:42Z

src/main/java/seedu/address/storage/XmlAddressBookStorage.java

        requireNonNull(addressBook);
        requireNonNull(filePath);

-        File file = new File(filePath);
+        File file = filePath.toFile();
        FileUtil.createIfMissing(file);


The FileUtil methods should also be modified to take in Paths as well?

pyokagan · 2018-03-31T03:16:10Z

src/test/java/seedu/address/commons/util/ConfigUtilTest.java

@@ -18,7 +19,7 @@

 public class ConfigUtilTest {

-    private static final String TEST_DATA_FOLDER = FileUtil.getPath("./src/test/data/ConfigUtilTest/");
+    private static final Path TEST_DATA_FOLDER = Paths.get(".", "src", "test", "data", "ConfigUtilTest");


The initial "." is unnecessary, can remove it.

pyokagan · 2018-03-31T03:18:08Z

src/test/java/seedu/address/storage/JsonUserPrefsStorageTest.java

 import seedu.address.model.UserPrefs;

 public class JsonUserPrefsStorageTest {

-    private static final String TEST_DATA_FOLDER = FileUtil.getPath("./src/test/data/JsonUserPrefsStorageTest/");
+    private static final Path TEST_DATA_FOLDER = Paths.get(".", "src", "test", "data", "JsonUserPrefsStorageTest");


Can remove the initial "."

pyokagan · 2018-03-31T03:18:35Z

src/test/java/seedu/address/storage/XmlAddressBookStorageTest.java

 import seedu.address.model.AddressBook;
 import seedu.address.model.ReadOnlyAddressBook;

 public class XmlAddressBookStorageTest {
-    private static final String TEST_DATA_FOLDER = FileUtil.getPath("./src/test/data/XmlAddressBookStorageTest/");
+    private static final Path TEST_DATA_FOLDER = Paths.get(".", "src", "test", "data", "XmlAddressBookStorageTest");


Can remove the initial "."

pyokagan · 2018-03-31T10:34:46Z

src/test/java/seedu/address/commons/util/ConfigUtilTest.java

        return config;
    }

-    private Optional<Config> read(String configFileInTestDataFolder) throws DataConversionException {
-        String configFilePath = addToTestDataPathIfNotNull(configFileInTestDataFolder);
+    private Optional<Config> read(Path configFileInTestDataFolder) throws DataConversionException {


Since this is a test method, it should be fine to either leave it alone taking in a String, or add an overload that takes in a String, so that we don't need to spam Paths.get() in all the tests. Being a test method, it is fine to get a InvalidPathException since it will fail the test anyway.

Same goes for all the other test methods.

CanIHasReview-bot · 2018-04-01T08:08:45Z

v2

@vivekscl submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/866/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

vivekscl · 2018-04-01T08:11:34Z

On a side note, I tried to place the conversion of all usages of File in the code base to Path as a separate commit but it almost always results in that commit failing tests since many of the method signatures were changed because of the conversion. So I placed it together with the first commit instead.

pyokagan

Some comments.

Do another sweep and see if you can use the java.nio.file.Files API instead of the java.io.File API. toFile() should be used only when absolutely required.

pyokagan · 2018-04-01T14:57:00Z

src/main/java/seedu/address/MainApp.java

-        return applicationParameters.get(parameterName);
+        return applicationParameters.get(parameterName) == null
+                ? Optional.empty()
+                : Optional.of(Paths.get(applicationParameters.get(parameterName)));


Doesn't this still cause the program to crash if the parameter is an invalid path?

It doesn't crash, but it does throw an exception and then starts the app like so:

But that's still bad practice, so I'll make the changes accordingly.

After trying to pass an invalid path to the master branch of this repo, the same error pops up and then the app starts, so it seems like this behaviour was already there from the beginning.

That's a NoSuchFileException. Paths.get only throws an InvalidPathException.

pyokagan · 2018-04-01T14:57:50Z

src/main/java/seedu/address/MainApp.java

@@ -75,9 +77,11 @@ public void init() throws Exception {
        initEventsCenter();
    }

-    private String getApplicationParameter(String parameterName) {
+    private Optional<Path> getApplicationParameter(String parameterName) {


Eh I wouldn't expect a method called getApplicationParameter to return a path. What happens when we want to support non-path parameters as well?

Ah, my bad. Should have studied the method more carefully 😅 .

pyokagan · 2018-04-01T14:58:26Z

src/main/java/seedu/address/MainApp.java

@@ -114,15 +118,15 @@ private void initLogging(Config config) {
     * The default file path {@code Config#DEFAULT_CONFIG_FILE} will be used instead
     * if {@code configFilePath} is null.
     */
-    protected Config initConfig(String configFilePath) {
+    protected Config initConfig(Optional<Path> configFilePath) {


We don't use Optional for optional arguments anymore. Use null instead.

pyokagan · 2018-04-01T15:00:25Z

src/main/java/seedu/address/commons/util/FileUtil.java

-    public static void createDirs(File dir) throws IOException {
-        if (!dir.exists() && !dir.mkdirs()) {
-            throw new IOException("Failed to make directories of " + dir.getName());
+    public static void createDirs(Path dir) throws IOException {


Seems like this whole function can be removed and replaced with Files.createDirectories()

pyokagan · 2018-04-01T15:01:04Z

src/main/java/seedu/address/commons/util/FileUtil.java


-        return file.createNewFile();
+        return filePath.toFile().createNewFile();


Seems like it can be replaced with Files.createFile()

CanIHasReview-bot · 2018-04-02T15:15:54Z

v3

@vivekscl submitted v3 for review.

[v3 1/2] Refactor codebase to use java.nio.file for file/path handling
[v3 2/2] FileUtil: remove unsued methods and FileUtilTest

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/866/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2018-04-03T08:39:29Z

src/main/java/seedu/address/MainApp.java

-        config = initConfig(getApplicationParameter("config"));
+        config = getApplicationParameter("config") != null
+                ? initConfig(Paths.get(getApplicationParameter("config")))
+                : initConfig(null);


Eh, this is smushing too many things into one line.

At least, I'd expect something like:

Path configPath = getApplicationParameter("config") != null ? Path.get(getApplicationParameter("config")) : null; config = initConfig(configPath);

(Operations on the configPath is separated from the config)

However, this code still does not handle the possible InvalidPathException from Paths.get(). You need something like:

String configPathParameter = getApplicationParameter("config"); if (configPathParameter != null && !isValidPath(configPathParameter)) { // Handle error // Considering the the rest of MainApp's code thinks its okay to not report errors to users, // I'm inclined to using something like: logger.warning("Invalid config path " + configPathParameter + ". Using default config path."); configPathParameter = null; } Path configPath = configPathParameter != null ? Path.get(configPathParameter) : null; config = initConfig(configPath);

Yeah, Java sucks.

For this, I tried testing if the InvalidPathException would be thrown using invalid paths as arguments, but it always returns true the isValidPath check on Linux. This SO post describes the same issue with Linux return true for any kind of path. Is it ok if I proceed with this check since it could work on Windows and MacOS?

private static boolean isValidPath(String path) { try { Paths.get(path); } catch (InvalidPathException ipe) { return false; } return true; }

@vivekscl

This SO post describes the same issue with Linux return true for any kind of path.

Seems like it has been fixed already.
https://ideone.com/4DONru

Is it ok if I proceed with this check since it could work on Windows and MacOS?

Yeah, of course. It's all about the API contract. If Paths.get claims that it could throw an InvalidPathException, then that needs to be handled, whether it does actually throw one or not.

pyokagan

[1/2]

pyokagan · 2018-04-03T14:15:03Z

src/main/java/seedu/address/commons/util/FileUtil.java

@@ -62,14 +62,15 @@ public static void createParentDirsOfFile(File file) throws IOException {

        if (parentDir != null) {
            createDirs(parentDir);
+            Files.createFile(file);


Hmm, why is this needed?

pyokagan · 2018-04-03T14:16:35Z

src/main/java/seedu/address/commons/util/FileUtil.java

@@ -88,6 +89,8 @@ public static void writeToFile(File file, String content) throws IOException {
    public static String getPath(String pathWithForwardSlash) {
        checkArgument(pathWithForwardSlash.contains("/"));
        return pathWithForwardSlash.replace("/", File.separator);
+    public static void writeToFile(Path filePath, String content) throws IOException {


Eh? No closing }? I'm surprised this compiles.

So, this compiles on the last commit because the problem is fixed in [2/2]. Do ensure that the project compiles + passes all tests on every commit.

pyokagan · 2018-04-03T14:17:36Z

src/main/java/seedu/address/commons/util/XmlUtil.java

@@ -18,53 +19,53 @@
    /**
     * Returns the xml data in the file as an object of the specified type.
     *
-     * @param file           Points to a valid xml file containing data that match the {@code classToConvert}.
+     * @param filePath       Points to a valid xml file path containing data that match the {@code classToConvert}.


Since it is "Points to a valid xml file", I guess there's no need to change the javadoc since a Path does also "Point" to a file?

pyokagan · 2018-04-03T14:17:45Z

src/main/java/seedu/address/commons/util/XmlUtil.java

    }

    /**
     * Saves the data in the file in xml format.
     *
-     * @param file Points to a valid xml file containing data that match the {@code classToConvert}.
-     *             Cannot be null.
+     * @param filePath Points to a valid xml file path containing data that match the {@code classToConvert}.


Since it is "Points to a valid xml file", I guess there's no need to change the javadoc since a Path does also "Point" to a file?

pyokagan · 2018-04-03T14:26:39Z

src/test/java/seedu/address/commons/util/ConfigUtilTest.java

@@ -75,12 +76,12 @@ private Config getTypicalConfig() {
        Config config = new Config();
        config.setAppTitle("Typical App Title");
        config.setLogLevel(Level.INFO);
-        config.setUserPrefsFilePath("C:\\preferences.json");
+        config.setUserPrefsFilePath(Paths.get("C:\\preferences.json"));


I just noticed that this string is technically just C:\preferences.json :-P

Anyway, even if it's C:\\preferences.json, such a Path only makes sense on Windows, and doesn't make sense for a test case which is meant to be run on different OS's.

Maybe add a commit on top to change this into a platform-independent path. Maybe just preferences.json will do.

pyokagan · 2018-04-03T14:29:25Z

src/test/java/seedu/address/ui/PersonListPanelTest.java

-        File manyPersonsFile = new File(TEST_DATA_FOLDER + "manyPersons.xml");
-        FileUtil.createFile(manyPersonsFile);
+        Path manyPersonsFile = TEST_DATA_FOLDER.resolve("manyPersons.xml");
+        Files.createFile(manyPersonsFile);


Would this fail if a file called manyPersons.xml happens to exist in src/test/data/sandbox? (if the cleanup fails for whatever reason previously?)

Maybe just leave it as FileUtil.createFile(manyPersonFile)?

pyokagan · 2018-04-03T15:08:30Z

src/test/java/seedu/address/commons/util/XmlUtilTest.java

@@ -122,7 +123,7 @@ public void saveDataToFile_missingFile_fileNotFoundException() throws Exception

    @Test
    public void saveDataToFile_validFile_dataSaved() throws Exception {
-        TEMP_FILE.createNewFile();
+        TEMP_FILE.toFile().createNewFile();


Files.createFile() looked like it could work here, except that it throws an exception if the file already exists.

Maybe use FileUtil.createFile()?

There are many usages of java.io.File throughout the codebase. Since Path is meant to replace File as mentioned in the Oracle Documentation on Legacy File I/O Code, let's convert all usages of File in the codebase to Path. Oracle Documentation on Legacy File I/O Code: https://docs.oracle.com/javase/tutorial/essential/io/legacy.html

CanIHasReview-bot · 2018-04-20T16:43:29Z

v13

@vivekscl submitted v13 for review.

[v13 1/6] Replace use of java.io.File with java.nio.file.Path
[v13 2/6] MainApp: extract handling of application parameters into AppParameters
[v13 3/6] Refactor codebase to use java.nio.file for file/path handling
[v13 4/6] FileUtil: use void for the return type of FileUtil#createFile(Path)
[v13 5/6] FileUtil: remove unused methods
[v13 6/6] ConfigUtilTest: use platform-independent path for preferences

(📚 Archive) (📈 Interdiff between v12 and v13)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/866/13/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

vivekscl · 2018-04-20T16:46:06Z

Merge commit message:

String operations are used for the construction of file paths. Also,
many APIs in the codebase take in String arguments as file paths.

This method of file/path handling is error prone as creating paths using
string concatenation can lead to the creation of invalid paths.
Furthermore, checking if two paths resolve to the same file using string
equality is incorrect as it only tests if the strings are equal.

Let's refactor the codebase to use java.nio.file for file/path
handling as it provides a safe and platform-independent way to
perform operations on paths and properly separate the concerns of
parsing/stringifying paths and manipulating paths.

More information on why String operations are error-prone:
http://twistedoakstudios.com/blog/Post4872_dont-treat-paths-like-strings

yamgent

[v13 2/6]

The handling of application parameters is done in the MainApp.

This decreases cohesiveness as handling and parsing of application
parameters should be done separately and not in the MainApp as
command-line parsing is at a much lower level compared to the high level
setup of the application architecture. This also allows the parsing of
application parameters to be tested independently.

Let's extract the handling of application parameters from MainApp into
the AppParameters for future parsing of application parameters.


Refactor codebase to use java.nio.file for file/path handling
---
String operations are used for the construction of file paths. Also,
many APIs in the codebase take in String arguments as file paths.

This method of file/path handling is error prone as creating paths using
string concatenation can lead to the creation of invalid paths.
Furthermore, checking if two paths resolve to the same file using string
equality is incorrect as it only tests if the strings are equal.

Let's refactor the codebase to use java.nio.file for file/path
handling as it provides a safe and platform-independent way to
perform operations on paths and properly separate the concerns of
parsing/stringifying paths and manipulating paths.

More information on why String operations are error-prone:
http://twistedoakstudios.com/blog/Post4872_dont-treat-paths-like-strings

Commit message contains the messages for both [2/6] and [3/6].

Other than that, no other major issues I can find.

The handling of application parameters is done in the MainApp. This decreases cohesiveness as handling and parsing of application parameters should be done separately and not in the MainApp as command-line parsing is at a much lower level compared to the high level setup of the application architecture. This also allows the parsing of application parameters to be tested independently. Let's extract the handling of application parameters from MainApp into the AppParameters for future parsing of application parameters.

String operations are used for the construction of file paths. Also, many APIs in the codebase take in String arguments as file paths. This method of file/path handling is error prone as creating paths using string concatenation can lead to the creation of invalid paths. Furthermore, checking if two paths resolve to the same file using string equality is incorrect as it only tests if the strings are equal. Let's refactor the codebase to use java.nio.file for file/path handling as it provides a safe and platform-independent way to perform operations on paths and properly separate the concerns of parsing/stringifying paths and manipulating paths. More information on why String operations are error-prone: http://twistedoakstudios.com/blog/Post4872_dont-treat-paths-like-strings

The return value of FileUtil#createFile(Path) is unused. For code quality purposes, methods with unused return types should have void as a return type instead. Let's use void for the return type of FileUtil#createFile(Path).

After refactoring the codebase to use java.nio.file for file/path handling, several methods became unused. Following that, FileUtilTest would be testing unused methods. For code quality purposes, unused methods and tests should be removed. Let's remove unused methods in FileUtil and FileUtilTest.

ConfigUtilTest#getTypicalConfig() uses a path for the user preference file that is only meant for the Windows operating system. This should not be the case as AddressBook is meant to run on all platforms. Let's update ConfigUtilTest#getTypicalConfig() to use a platform-independent path for the user preference file. Following this, ExtraValuesConfig.json and TypicalConfig.json should be updated to reflect this change as well.

pyokagan · 2018-04-21T09:02:49Z

@yamgent Oh that's bad, thanks for the sharp eyes.

vivekscl · 2018-04-21T09:03:50Z

Commit message contains the messages for both [2/6] and [3/6].

@yamgent thanks for catching that! Really bad mistake on my part 😅 .

CanIHasReview-bot · 2018-04-21T09:12:46Z

v14

@vivekscl submitted v14 for review.

[v14 1/6] Replace use of java.io.File with java.nio.file.Path
[v14 2/6] MainApp: extract handling of application parameters into AppParameters
[v14 3/6] Refactor codebase to use java.nio.file for file/path handling
[v14 4/6] FileUtil: use void for the return type of FileUtil#createFile(Path)
[v14 5/6] FileUtil: remove unused methods
[v14 6/6] ConfigUtilTest: use platform-independent path for preferences

(📚 Archive) (📈 Interdiff between v13 and v14)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/866/14/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-04-21T10:04:21Z

@vivekscl you can last call review this.

vivekscl · 2018-04-22T03:51:52Z

@yamgent I guess this is ready to merge? 😄

se-edu deleted a comment Mar 28, 2018

pyokagan suggested changes Mar 29, 2018

View reviewed changes

vivekscl force-pushed the 750-use-java.nio.file branch 2 times, most recently from c4ef491 to ec1e15d Compare March 30, 2018 19:21

CanIHasReview-bot added the s.ToReview label Mar 30, 2018

CanIHasReview-bot requested a review from pyokagan March 30, 2018 19:33

pyokagan suggested changes Mar 31, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Mar 31, 2018

vivekscl force-pushed the 750-use-java.nio.file branch 2 times, most recently from 273f993 to fb665f3 Compare April 1, 2018 07:53

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 1, 2018

CanIHasReview-bot requested a review from pyokagan April 1, 2018 08:08

pyokagan suggested changes Apr 1, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Apr 1, 2018

Zhiyuan-Amos requested a review from yamidark April 2, 2018 12:00

vivekscl force-pushed the 750-use-java.nio.file branch from fb665f3 to 52b8383 Compare April 2, 2018 14:52

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 2, 2018

CanIHasReview-bot requested a review from pyokagan April 2, 2018 15:15

pyokagan reviewed Apr 3, 2018

View reviewed changes

pyokagan suggested changes Apr 3, 2018

View reviewed changes

vivekscl force-pushed the 750-use-java.nio.file branch from dc624a1 to f46c111 Compare April 20, 2018 16:31

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 20, 2018

CanIHasReview-bot requested a review from Zhiyuan-Amos April 20, 2018 16:43

Zhiyuan-Amos approved these changes Apr 20, 2018

View reviewed changes

yamgent reviewed Apr 21, 2018

View reviewed changes

pyokagan removed the s.ToReview label Apr 21, 2018

vivekscl added 5 commits April 21, 2018 17:01

vivekscl force-pushed the 750-use-java.nio.file branch from f46c111 to 50771f4 Compare April 21, 2018 09:02

pyokagan added the s.Ongoing label Apr 21, 2018

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 21, 2018

yamgent approved these changes Apr 21, 2018

View reviewed changes

yamgent requested a review from damithc April 21, 2018 09:37

damithc approved these changes Apr 21, 2018

View reviewed changes

yamgent added s.ToMerge and removed s.ToReview labels Apr 21, 2018

yamgent merged commit e968a29 into se-edu:master Apr 22, 2018

		@@ -116,13 +118,13 @@ private void initLogging(Config config) {
		*/
		protected Config initConfig(String configFilePath) {


		return file.createNewFile();
		return filePath.toFile().createNewFile();

Use java.nio.file for file/path handling #750 #866

Use java.nio.file for file/path handling #750 #866

Conversation

vivekscl commented Mar 28, 2018 • edited by pyokagan Loading

Things to take note of

Issues to raise

pyokagan left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 30, 2018

v1

pyokagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 1, 2018

v2

vivekscl commented Apr 1, 2018

pyokagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 2, 2018

v3

Choose a reason for hiding this comment

vivekscl Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 20, 2018

v13

vivekscl commented Apr 20, 2018

yamgent left a comment • edited Loading

Choose a reason for hiding this comment

pyokagan commented Apr 21, 2018

vivekscl commented Apr 21, 2018

CanIHasReview-bot commented Apr 21, 2018

v14

yamgent commented Apr 21, 2018

vivekscl commented Apr 22, 2018

vivekscl commented Mar 28, 2018 •

edited by pyokagan

Loading

vivekscl Apr 4, 2018 •

edited

Loading

yamgent left a comment •

edited

Loading