test #1

Eclipse-Dominator · 2023-05-22T06:03:15Z

DUMMY ASDASDasdfadsf

Eclipse-Dominator · 2023-05-22T06:33:45Z

test1234

Eclipse-Dominator · 2023-05-22T07:02:45Z

2023-05-09 16:21:03:

damithc commented on 138

    >Putting this hold for now.

    </details>

2023-05-09 16:26:04:

damithc commented on 153

    >```java

@JsonProperty("tagged") List tagged) {


I think it is best if we modify both occurrences of `tagged`, if possible (or else we introduce an inconsistency which can be argued as even worse than the naming anomaly being fixed). So, I would like to explore that option before merging this PR.

        </details>
        

        

---
2023-05-09 16:36:20: 
        <details>
        <summary>damithc commented on [152](https://github.com/se-edu/addressbook-level3/pull/152)</summary>

        >Sorry, it took a while for me to come back to this PR.
I'm wondering if including the target filename in the log message is better (rather than the current `Prefs file not found. ...`, `Preferences file prefs.json not found. ...`)

        </details>
        

        

---
2023-05-11 13:55:28: [#147] Rename tagged to tags was created by Eclipse-Dominator

---
2023-05-11 13:55:31: 
        <details>
        <summary>canihasreview commented on [163](https://github.com/se-edu/addressbook-level3/pull/163)</summary>

        >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/163) when this PR is ready for review.

See this repository's [contribution guide](https://oss-generic.github.io/process/docs/QuestionsIssuesPrs.html#submitting-a-pr) for more information.

        </details>
        

        

---
2023-05-11 15:16:46: [#139] Rephrase 'file not found' logs shown during first run was created by Eclipse-Dominator

---
2023-05-11 15:16:49: 
        <details>
        <summary>canihasreview commented on [164](https://github.com/se-edu/addressbook-level3/pull/164)</summary>

        >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/164) when this PR is ready for review.

See this repository's [contribution guide](https://oss-generic.github.io/process/docs/QuestionsIssuesPrs.html#submitting-a-pr) for more information.

        </details>
        

        

---
2023-05-11 15:26:25: 
        <details>
        <summary>Eclipse-Dominator commented on [164](https://github.com/se-edu/addressbook-level3/pull/164)</summary>

        >While working on this, I realised that the 
```java
            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

    </details>

2023-05-11 15:52:06: [#73] no write access fail gracefully was created by Eclipse-Dominator

2023-05-11 15:52:10:

canihasreview commented on 165

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/165) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-11 15:56:22:

Eclipse-Dominator commented on 165

    >There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

    </details>

2023-05-11 16:14:42:

Eclipse-Dominator edited comment on 165

    >There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

    </details>

2023-05-11 17:00:45: Rename tagged to tags was created by Eclipse-Dominator

2023-05-11 17:00:48:

canihasreview commented on 168

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/168) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-11 17:15:09: [#147] rename tagged to tags was created by Eclipse-Dominator

2023-05-11 17:15:12:

canihasreview commented on 169

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/169) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-11 17:15:27:

canihasreview commented on 169

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v1 2/2] Change json property from tagged to tags

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-11 17:28:22: [#139] Rephrase 'file not found' during first run was created by Eclipse-Dominator

2023-05-11 17:28:26:

canihasreview commented on 170

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/170) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-11 17:29:14:

Eclipse-Dominator commented on 170

    >While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I have made a separate issue regarding this: #167

    </details>

2023-05-11 17:29:29:

canihasreview commented on 170

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/4] Add logs for creation of default files
[v1 2/4] Change file not found log to file found
[v1 3/4] Modify warning to show file location
[v1 4/4] Adds more file paths to warnings

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-11 17:30:05:

Eclipse-Dominator edited comment on 164

    >While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

    </details>

2023-05-11 17:31:06:

Eclipse-Dominator commented on 164

    >use #170 

    </details>

2023-05-11 20:04:43: [#73] no write access fail gracefully was created by Eclipse-Dominator

2023-05-11 20:04:47:

canihasreview commented on 172

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/172) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-11 20:13:39:

Eclipse-Dominator commented on 165

    >Refer to #172 

    </details>

2023-05-11 23:23:43:

canihasreview commented on 172

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/3] Improve error message if no write permission
[v1 2/3] Adds separate log for permission denied error
[v1 3/3] Adds back invalid command log

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-12 04:26:16:

codecov-commenter commented on 169

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/169?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-12 09:56:44:

canihasreview commented on 169

    ># v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v2 2/2] JsonAdaptedPerson: Use tags for JSON property (#169)

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

    </details>

2023-05-12 09:58:04:

Eclipse-Dominator commented on 169

    >I have updated the commit to include a commit body. I did do some testing with changes and no issues were spotted. 

    </details>

2023-05-12 10:29:45:

canihasreview commented on 170

    ># v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

    </details>

2023-05-12 10:32:49:

canihasreview commented on 170

    ># v3

@Eclipse-Dominator submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

    </details>

2023-05-12 11:02:00:

canihasreview commented on 172

    ># v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

    </details>

2023-05-12 11:06:03:

Eclipse-Dominator commented on 172

    >Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

    </details>

2023-05-12 11:06:10:

Eclipse-Dominator edited comment on 172

    >Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

    </details>

2023-05-12 11:08:21:

canihasreview commented on 170

    ># v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/3] Add logs for creation of default files
[v4 2/3] Change file not found log to file found
[v4 3/3] MainApp: Modify warning to show file location

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/4/head:BRANCHNAME

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

    </details>

2023-05-12 11:18:23: [#173] Null class logger not logging to logger output was created by Eclipse-Dominator

2023-05-12 11:18:26:

canihasreview commented on 174

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/174) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-12 13:36:34:

canihasreview commented on 169

    ># v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

    </details>

2023-05-12 13:36:36:

canihasreview commented on 169

    ># v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

    </details>

2023-05-12 13:48:23:

canihasreview commented on 170

    ># v5

@Eclipse-Dominator submitted v5 for review.

[v5 1/3] Add logs for creation of default files
[v5 2/3] Change file not found log to file found
[v5 3/3] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/5/head:BRANCHNAME

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

    </details>

2023-05-12 13:51:18:

Eclipse-Dominator commented on 169

    >Yup I just realized it as well.

    </details>

2023-05-12 13:54:45:

canihasreview commented on 169

    ># v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/2] JsonAdaptedPerson: Change variable name tagged -> tags
[v4 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/169/4/head:BRANCHNAME

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

    </details>

2023-05-12 13:58:43:

codecov-commenter edited comment on 169

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/169?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-12 14:08:28:

damithc commented on 152

    >Closing in favor of #170

    </details>

2023-05-12 14:09:32:

damithc commented on 169

    >Thanks to @Py0000 for your contribution to this PR too.

    </details>

2023-05-12 14:38:00:

codecov-commenter commented on 174

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/174?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-12 17:41:57:

canihasreview commented on 170

    ># v6

@Eclipse-Dominator submitted v6 for review.

[v6 1/2] Add logs for creation and discovery of default files
[v6 2/2] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/6/head:BRANCHNAME

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

    </details>

2023-05-13 23:34:03:

Eclipse-Dominator commented on 170

    >I have modified and standardized some logs. Regarding the use of full stop, adding full stop to certain messagaes in the format like `"TXT" + filename` can feel rather messy, I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

    </details>

2023-05-14 04:38:54:

damithc commented on 170

    >>I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

@Eclipse-Dominator Yes, this PR should not touch other log messages. But the log messages touched in this PR should be locally consistent (i.e., consistent with the surrounding code).

    </details>

2023-05-14 05:17:26:

canihasreview commented on 170

    ># v7

@Eclipse-Dominator submitted v7 for review.

[v7 1/2] Make file-creation logs not resemble errors
[v7 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/7/head:BRANCHNAME

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

    </details>

2023-05-14 05:47:03:

damithc commented on 153

    >Fixed by #169 that builds on this PR.

Thanks again for this PR @Py0000

    </details>

2023-05-14 05:48:03:

damithc commented on 77

    >To be continued by #172

Thanks for this PR @ernestlim8

    </details>

2023-05-14 09:05:54:

canihasreview commented on 170

    ># v8

@Eclipse-Dominator submitted v8 for review.

[v8 1/2] Make file-creation logs not resemble errors
[v8 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v7 and v8) (📈 Range-Diff between v7 and v8)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/8/head:BRANCHNAME

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

    </details>

2023-05-14 09:47:01:

canihasreview commented on 170

    ># v9

@Eclipse-Dominator submitted v9 for review.

[v9 1/2] Make file-creation logs not resemble errors
[v9 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v8 and v9) (📈 Range-Diff between v8 and v9)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/9/head:BRANCHNAME

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

    </details>

2023-05-14 15:00:15:

canihasreview commented on 170

    ># v10

@Eclipse-Dominator submitted v10 for review.

[v10 1/2] Make file-creation logs not resemble errors
[v10 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v9 and v10) (📈 Range-Diff between v9 and v10)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/10/head:BRANCHNAME

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

    </details>

2023-05-14 15:08:42: [#171] Updating gitignore to account for bin folder was created by Eclipse-Dominator

2023-05-14 15:08:44:

canihasreview commented on 175

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/175) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-14 15:10:40:

codecov-commenter commented on 175

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/175?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-14 15:10:58:

canihasreview commented on 175

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add bin/ folder to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-14 15:11:36:

codecov-commenter edited comment on 175

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/175?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-14 15:38:37:

damithc commented on 170

    >@se-edu/tech-team-level1 for your review, if any of you are available these days.

Please review each commit independently, including the commit message. Ensure the diff matches the commit message. Don't hesitate to nitpick, as we want to polish up each commit as much as we can, before merging.

    </details>

2023-05-14 16:12:24:

damithc commented on 175

    >I could not reproduce this behavior on Windows i.e., I ran `gradlew` but it did not create a `bin` folder. 🤔

Commit message: generated -> generates?

    </details>

2023-05-15 07:19:32:

damithc commented on 172

    >> There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

hmm... there is no real need to save data upon exit. Perhaps we can skip the data saving if the command is the exit command?

    </details>

2023-05-15 07:49:25:

Eclipse-Dominator commented on 175

    >After some investigation, the creation of `bin/` folder seems to be caused by a separate vscode extension. As the focus of the module is the use of Intellij, let's close this for now.

    </details>

2023-05-15 08:10:35:

canihasreview commented on 170

    ># v11

@Eclipse-Dominator submitted v11 for review.

[v11 1/2] Make file-creation logs not resemble errors
[v11 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/11/head:BRANCHNAME

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

    </details>

2023-05-16 03:07:39:

Eclipse-Dominator commented on 172

    >Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

    </details>

2023-05-16 03:10:21:

Eclipse-Dominator edited comment on 172

    >Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

    </details>

2023-05-16 04:24:50:

codecov-commenter commented on 172

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-16 04:53:46:

damithc commented on 172

    >As this is a very rare case, we should try a solution that minimizes changes to the code. So, approach 3 should be fine.

Do you think this PR can be just one commit?

    </details>

2023-05-16 05:27:24:

Eclipse-Dominator commented on 172

    >I think 2 commit is probably more suited here?

Decoupling IOException and AccessDeniedException
Decoupling ParseException and CommandException

As for the commit body, I am thinking of

Better error message when file have no write-access

When the data file has no write access, a message containing.
a raw exception is displayed to the user with no specific instructions
on handling such errors.

Let's 
* Polish up error messages displayed to the user
* Decouple IOExceptions and AccessDeniedExceptions
* Add instructions to ask the users to manually close AB3

Decoupling of ParseException and CommandException log messages

All errors related to command is logged as "invalid command".
Let's be more specific by decoupling ParseExceptions and CommandExceptions
to enable an more accurate logging of the errors.

    </details>

2023-05-16 05:54:22:

damithc commented on 172

    >I'm not sure if the second commit adds enough value. While the same prefix is used in both cases, the error message itself should tell us what exactly happened, right?

Don't forget that commit message subject should be in imperative mood.

    </details>

2023-05-17 05:25:22:

canihasreview commented on 170

    ># v12

@Eclipse-Dominator submitted v12 for review.

[v12 1/2] Make file-creation logs not resemble errors
[v12 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/12/head:BRANCHNAME

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

    </details>

2023-05-17 05:27:03:

Eclipse-Dominator commented on 170

    >Updated iteration fixing the line breaks, please update if there is any more changes required @se-edu/tech-team-level1

    </details>

2023-05-17 21:02:34: [#162] Show warning when ignoring repeated parameters was created by Eclipse-Dominator

2023-05-17 21:02:37:

canihasreview commented on 176

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/176) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-17 21:03:09:

canihasreview commented on 176

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Show warning when ignoring repeated parameters

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-17 21:09:11:

codecov-commenter commented on 176

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/176?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #176 (ce23ddf) into master (1e05fef) will decrease coverage by 0.23%.
The diff coverage is 58.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
- Coverage     72.15%   71.93%   -0.23%     
- Complexity      399      402       +3     
============================================
  Files            70       70              
  Lines          1232     1247      +15     
  Branches        125      131       +6     
============================================
+ Hits            889      897       +8     
- Misses          311      312       +1     
- Partials         32       38       +6

Impacted Files	Coverage Δ
.../seedu/address/logic/parser/EditCommandParser.java	`81.25% <33.33%> (-11.06%)`	⬇️
...java/seedu/address/logic/commands/EditCommand.java	`93.42% <72.72%> (-3.60%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-18 05:36:18: [#166] Bump javafx to version 11.0.2 was created by Eclipse-Dominator

2023-05-18 05:36:21:

canihasreview commented on 177

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/177) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-18 05:38:48:

canihasreview commented on 177

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] Fix JavaFx compatibility issue
[v1 2/2] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-18 09:18:37:

Eclipse-Dominator commented on 176

    >Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

    </details>

2023-05-18 09:52:48:

damithc commented on 176

    >> Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

Yup, this may still be the best option but it's better to explore other options before making a decision.

    </details>

2023-05-18 11:44:23: [#178] Include JVM Crash Logs in gitignore was created by Eclipse-Dominator

2023-05-18 11:44:26:

canihasreview commented on 179

    >[Click here to submit a new iteration](https://canihasreview.pyokagan.com/se-edu/addressbook-level3/pull/179) when this PR is ready for review.

See this repository's contribution guide for more information.

    </details>

2023-05-18 11:46:22:

canihasreview commented on 179

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-18 11:47:14:

codecov-commenter commented on 179

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/179?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-18 11:48:55:

codecov-commenter edited comment on 179

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/179?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-18 11:53:23:

canihasreview commented on 177

    ># v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Fix JavaFx compatibility issue

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

    </details>

2023-05-18 11:55:09:

codecov-commenter commented on 177

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/177?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-18 11:56:36:

codecov-commenter edited comment on 177

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/177?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-18 12:04:32:

canihasreview commented on 174

    ># v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add assertion for null class loggers

(📚 Archive)

Checkout this PR version locally

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

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

    </details>

2023-05-18 13:34:52:

damithc commented on 179

    >Thanks for the PR @Eclipse-Dominator

I felt the change is simple enough to warrant a subject-only commit message, so I took the liberty of shortening it.

    </details>

2023-05-18 13:53:39:

damithc commented on 177

    >@zhongfu I remember you did some investigation into this problem earlier. If you are free, your inputs are welcome.

    </details>

2023-05-19 12:01:04:

Eclipse-Dominator commented on 172

    >I have updated the commit to the following:

Improve error message if no write permission

Insufficient permission to read the data file will show
an AccessDeniedException as error message.
AB3 should be able to fail gracefully with instructions
to close the application upon failure.

Let's
- make the error message more user friendly by informing
the user that there are insufficient permissions for the file.
- Decouple AccessDeniedException from IOException messages.
- Decouple ParseException and CommandException log messages.

    </details>

2023-05-19 12:03:23:

canihasreview commented on 174

    ># v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Add assertion for null class loggers

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

    </details>

2023-05-20 00:50:09:

Eclipse-Dominator commented on 174

    >I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.


    </details>

2023-05-20 00:50:27:

canihasreview commented on 174

    ># v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

    </details>

2023-05-20 00:51:24:

codecov edited comment on 174

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/174?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

2023-05-20 04:23:08:

codecov edited comment on 172

    >## [Codecov](https://app.codecov.io/gh/se-edu/addressbook-level3/pull/172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=se-edu) Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    </details>

Eclipse-Dominator · 2023-05-22T07:06:41Z

2023-05-09 16:21:03:
@damithc commented on 138

View details

Putting this hold for now.

2023-05-09 16:26:04:
damithc commented on 153

View details

@JsonProperty("tagged") List<JsonAdaptedTag> tagged) {

I think it is best if we modify both occurrences of tagged, if possible (or else we introduce an inconsistency which can be argued as even worse than the naming anomaly being fixed). So, I would like to explore that option before merging this PR.

2023-05-09 16:36:20:
damithc commented on 152

View details

Sorry, it took a while for me to come back to this PR.
I'm wondering if including the target filename in the log message is better (rather than the current Prefs file not found. ..., Preferences file prefs.json not found. ...)

2023-05-11 13:55:28: [#147] Rename tagged to tags was created by Eclipse-Dominator

2023-05-11 13:55:31:
canihasreview commented on 163

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:16:46: [#139] Rephrase 'file not found' logs shown during first run was created by Eclipse-Dominator

2023-05-11 15:16:49:
canihasreview commented on 164

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:26:25:
Eclipse-Dominator commented on 164

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

2023-05-11 15:52:06: [#73] no write access fail gracefully was created by Eclipse-Dominator

2023-05-11 15:52:10:
canihasreview commented on 165

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:56:22:
Eclipse-Dominator commented on 165

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

2023-05-11 16:14:42:
Eclipse-Dominator edited comment on 165

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

2023-05-11 17:00:45: Rename tagged to tags was created by Eclipse-Dominator

2023-05-11 17:00:48:
canihasreview commented on 168

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:15:09: [#147] rename tagged to tags was created by Eclipse-Dominator

2023-05-11 17:15:12:
canihasreview commented on 169

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:15:27:
canihasreview commented on 169

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v1 2/2] Change json property from tagged to tags

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-11 17:28:22: [#139] Rephrase 'file not found' during first run was created by Eclipse-Dominator

2023-05-11 17:28:26:
canihasreview commented on 170

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:29:14:
Eclipse-Dominator commented on 170

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I have made a separate issue regarding this: #167

2023-05-11 17:29:29:
canihasreview commented on 170

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/4] Add logs for creation of default files
[v1 2/4] Change file not found log to file found
[v1 3/4] Modify warning to show file location
[v1 4/4] Adds more file paths to warnings

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-11 17:30:05:
Eclipse-Dominator edited comment on 164

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

2023-05-11 17:31:06:
Eclipse-Dominator commented on 164

View details

use #170

2023-05-11 20:04:43: [#73] no write access fail gracefully was created by Eclipse-Dominator

2023-05-11 20:04:47:
canihasreview commented on 172

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 20:13:39:
Eclipse-Dominator commented on 165

View details

Refer to #172

2023-05-11 23:23:43:
canihasreview commented on 172

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/3] Improve error message if no write permission
[v1 2/3] Adds separate log for permission denied error
[v1 3/3] Adds back invalid command log

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-12 04:26:16:
codecov-commenter commented on 169

View details

Codecov Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 09:56:44:
canihasreview commented on 169

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v2 2/2] JsonAdaptedPerson: Use tags for JSON property (#169)

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 09:58:04:
Eclipse-Dominator commented on 169

View details

I have updated the commit to include a commit body. I did do some testing with changes and no issues were spotted.

2023-05-12 10:29:45:
canihasreview commented on 170

View details

v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 10:32:49:
canihasreview commented on 170

View details

v3

@Eclipse-Dominator submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 11:02:00:
canihasreview commented on 172

View details

v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 11:06:03:
Eclipse-Dominator commented on 172

View details

Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

2023-05-12 11:06:10:
Eclipse-Dominator edited comment on 172

View details

Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

2023-05-12 11:08:21:
canihasreview commented on 170

View details

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/3] Add logs for creation of default files
[v4 2/3] Change file not found log to file found
[v4 3/3] MainApp: Modify warning to show file location

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/4/head:BRANCHNAME

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

2023-05-12 11:18:23: [#173] Null class logger not logging to logger output was created by Eclipse-Dominator

2023-05-12 11:18:26:
canihasreview commented on 174

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-12 13:36:34:
canihasreview commented on 169

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 13:36:36:
canihasreview commented on 169

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 13:48:23:
canihasreview commented on 170

View details

v5

@Eclipse-Dominator submitted v5 for review.

[v5 1/3] Add logs for creation of default files
[v5 2/3] Change file not found log to file found
[v5 3/3] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/5/head:BRANCHNAME

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

2023-05-12 13:51:18:
Eclipse-Dominator commented on 169

View details

Yup I just realized it as well.

2023-05-12 13:54:45:
canihasreview commented on 169

View details

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/2] JsonAdaptedPerson: Change variable name tagged -> tags
[v4 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/169/4/head:BRANCHNAME

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

2023-05-12 13:58:43:
codecov-commenter edited comment on 169

View details

Codecov Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 14:08:28:
damithc commented on 152

View details

Closing in favor of #170

2023-05-12 14:09:32:
damithc commented on 169

View details

Thanks to @Py0000 for your contribution to this PR too.

2023-05-12 14:38:00:
codecov-commenter commented on 174

View details

Codecov Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 17:41:57:
canihasreview commented on 170

View details

v6

@Eclipse-Dominator submitted v6 for review.

[v6 1/2] Add logs for creation and discovery of default files
[v6 2/2] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/6/head:BRANCHNAME

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

2023-05-13 23:34:03:
Eclipse-Dominator commented on 170

View details

I have modified and standardized some logs. Regarding the use of full stop, adding full stop to certain messagaes in the format like "TXT" + filename can feel rather messy, I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

2023-05-14 04:38:54:
damithc commented on 170

View details

I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

@Eclipse-Dominator Yes, this PR should not touch other log messages. But the log messages touched in this PR should be locally consistent (i.e., consistent with the surrounding code).

2023-05-14 05:17:26:
canihasreview commented on 170

View details

v7

@Eclipse-Dominator submitted v7 for review.

[v7 1/2] Make file-creation logs not resemble errors
[v7 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/7/head:BRANCHNAME

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

2023-05-14 05:47:03:
damithc commented on 153

View details

Fixed by #169 that builds on this PR.
Thanks again for this PR @Py0000

2023-05-14 05:48:03:
damithc commented on 77

View details

To be continued by #172
Thanks for this PR @ernestlim8

2023-05-14 09:05:54:
canihasreview commented on 170

View details

v8

@Eclipse-Dominator submitted v8 for review.

[v8 1/2] Make file-creation logs not resemble errors
[v8 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v7 and v8) (📈 Range-Diff between v7 and v8)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/8/head:BRANCHNAME

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

2023-05-14 09:47:01:
canihasreview commented on 170

View details

v9

@Eclipse-Dominator submitted v9 for review.

[v9 1/2] Make file-creation logs not resemble errors
[v9 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v8 and v9) (📈 Range-Diff between v8 and v9)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/9/head:BRANCHNAME

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

2023-05-14 15:00:15:
canihasreview commented on 170

View details

v10

@Eclipse-Dominator submitted v10 for review.

[v10 1/2] Make file-creation logs not resemble errors
[v10 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v9 and v10) (📈 Range-Diff between v9 and v10)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/10/head:BRANCHNAME

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

2023-05-14 15:08:42: [#171] Updating gitignore to account for bin folder was created by Eclipse-Dominator

2023-05-14 15:08:44:
canihasreview commented on 175

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-14 15:10:40:
codecov-commenter commented on 175

View details

Codecov Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-14 15:10:58:
canihasreview commented on 175

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add bin/ folder to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-14 15:11:36:
codecov-commenter edited comment on 175

View details

Codecov Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-14 15:38:37:
damithc commented on 170

View details

@se-edu/tech-team-level1 for your review, if any of you are available these days.
Please review each commit independently, including the commit message. Ensure the diff matches the commit message. Don't hesitate to nitpick, as we want to polish up each commit as much as we can, before merging.

2023-05-14 16:12:24:
damithc commented on 175

View details

I could not reproduce this behavior on Windows i.e., I ran gradlew but it did not create a bin folder. 🤔

Commit message: generated -> generates?

2023-05-15 07:19:32:
damithc commented on 172

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

hmm... there is no real need to save data upon exit. Perhaps we can skip the data saving if the command is the exit command?

2023-05-15 07:49:25:
Eclipse-Dominator commented on 175

View details

After some investigation, the creation of bin/ folder seems to be caused by a separate vscode extension. As the focus of the module is the use of Intellij, let's close this for now.

2023-05-15 08:10:35:
canihasreview commented on 170

View details

v11

@Eclipse-Dominator submitted v11 for review.

[v11 1/2] Make file-creation logs not resemble errors
[v11 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/11/head:BRANCHNAME

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

2023-05-16 03:07:39:
Eclipse-Dominator commented on 172

View details

Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

2023-05-16 03:10:21:
Eclipse-Dominator edited comment on 172

View details

Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

2023-05-16 04:24:50:
codecov-commenter commented on 172

View details

Codecov Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-16 04:53:46:
damithc commented on 172

View details

As this is a very rare case, we should try a solution that minimizes changes to the code. So, approach 3 should be fine.
Do you think this PR can be just one commit?

2023-05-16 05:27:24:
Eclipse-Dominator commented on 172

View details

I think 2 commit is probably more suited here?

Decoupling IOException and AccessDeniedException
Decoupling ParseException and CommandException

As for the commit body, I am thinking of

Better error message when file have no write-access

When the data file has no write access, a message containing.
a raw exception is displayed to the user with no specific instructions
on handling such errors.

Let's 
* Polish up error messages displayed to the user
* Decouple IOExceptions and AccessDeniedExceptions
* Add instructions to ask the users to manually close AB3

Decoupling of ParseException and CommandException log messages

All errors related to command is logged as "invalid command".
Let's be more specific by decoupling ParseExceptions and CommandExceptions
to enable an more accurate logging of the errors.

2023-05-16 05:54:22:
damithc commented on 172

View details

I'm not sure if the second commit adds enough value. While the same prefix is used in both cases, the error message itself should tell us what exactly happened, right?
Don't forget that commit message subject should be in imperative mood.

2023-05-17 05:25:22:
canihasreview commented on 170

View details

v12

@Eclipse-Dominator submitted v12 for review.

[v12 1/2] Make file-creation logs not resemble errors
[v12 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/12/head:BRANCHNAME

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

2023-05-17 05:27:03:
Eclipse-Dominator commented on 170

View details

Updated iteration fixing the line breaks, please update if there is any more changes required @se-edu/tech-team-level1

2023-05-17 21:02:34: [#162] Show warning when ignoring repeated parameters was created by Eclipse-Dominator

2023-05-17 21:02:37:
canihasreview commented on 176

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-17 21:03:09:
canihasreview commented on 176

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Show warning when ignoring repeated parameters

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-17 21:09:11:
codecov-commenter commented on 176

View details

Codecov Report

Merging #176 (ce23ddf) into master (1e05fef) will decrease coverage by 0.23%.
The diff coverage is 58.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
- Coverage     72.15%   71.93%   -0.23%     
- Complexity      399      402       +3     
============================================
  Files            70       70              
  Lines          1232     1247      +15     
  Branches        125      131       +6     
============================================
+ Hits            889      897       +8     
- Misses          311      312       +1     
- Partials         32       38       +6

Impacted Files	Coverage Δ
.../seedu/address/logic/parser/EditCommandParser.java	`81.25% <33.33%> (-11.06%)`	⬇️
...java/seedu/address/logic/commands/EditCommand.java	`93.42% <72.72%> (-3.60%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 05:36:18: [#166] Bump javafx to version 11.0.2 was created by Eclipse-Dominator

2023-05-18 05:36:21:
canihasreview commented on 177

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-18 05:38:48:
canihasreview commented on 177

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] Fix JavaFx compatibility issue
[v1 2/2] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 09:18:37:
Eclipse-Dominator commented on 176

View details

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

2023-05-18 09:52:48:
damithc commented on 176

View details

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

Yup, this may still be the best option but it's better to explore other options before making a decision.

2023-05-18 11:44:23: [#178] Include JVM Crash Logs in gitignore was created by Eclipse-Dominator

2023-05-18 11:44:26:
canihasreview commented on 179

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-18 11:46:22:
canihasreview commented on 179

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 11:47:14:
codecov-commenter commented on 179

View details

Codecov Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:48:55:
codecov-commenter edited comment on 179

View details

Codecov Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:53:23:
canihasreview commented on 177

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Fix JavaFx compatibility issue

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-18 11:55:09:
codecov-commenter commented on 177

View details

Codecov Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:56:36:
codecov-commenter edited comment on 177

View details

Codecov Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 12:04:32:
canihasreview commented on 174

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add assertion for null class loggers

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 13:34:52:
damithc commented on 179

View details

Thanks for the PR @Eclipse-Dominator
I felt the change is simple enough to warrant a subject-only commit message, so I took the liberty of shortening it.

2023-05-18 13:53:39:
damithc commented on 177

View details

@zhongfu I remember you did some investigation into this problem earlier. If you are free, your inputs are welcome.

2023-05-19 12:01:04:
Eclipse-Dominator commented on 172

View details

I have updated the commit to the following:

Improve error message if no write permission

Insufficient permission to read the data file will show
an AccessDeniedException as error message.
AB3 should be able to fail gracefully with instructions
to close the application upon failure.

Let's
- make the error message more user friendly by informing
the user that there are insufficient permissions for the file.
- Decouple AccessDeniedException from IOException messages.
- Decouple ParseException and CommandException log messages.

2023-05-19 12:03:23:
canihasreview commented on 174

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Add assertion for null class loggers

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-20 00:50:09:
Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
canihasreview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-20 00:51:24:
codecov edited comment on 174

View details

Codecov Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-20 04:23:08:
codecov edited comment on 172

View details

Codecov Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Eclipse-Dominator · 2023-05-22T07:14:56Z

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

Eclipse-Dominator · 2023-05-22T07:17:21Z

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

Eclipse-Dominator · 2023-05-22T07:22:54Z

view digest

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

Eclipse-Dominator · 2023-05-22T07:24:34Z

Digest 2023-05-22T07:24:35Z

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

Eclipse-Dominator · 2023-05-22T07:25:44Z

Digest 2023-05-22T07:25:45Z

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

Eclipse-Dominator · 2023-05-22T07:25:46Z

Digest 2023-05-22T07:25:47Z

Eclipse-Dominator · 2023-05-22T07:34:18Z

Digest 2023-05-22T07:34:19Z

2023-01-27 16:26:11:
@damithc commented on 153

View details

Also, we need a full commit message as per our usual practice.

2023-01-27 16:29:03:
@damithc commented on 152

View details

@se-edu/tech-team-level1 for your review ...

2023-01-31 04:22:49: 2023-01-31 04:22:49: Bump activesupport from 6.0.6 to 6.0.6.1 in /docs was created by @dependabot: 155

2023-01-31 04:22:52:
@CanIHasReview commented on 155

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-01-31 04:24:49:
@codecov-commenter commented on 155

View details

Codecov Report

Merging #155 (f5aef85) into master (412b5ed) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##             master     #155   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-01-31 06:25:57:
@Py0000 commented on 153

View details

Also, we need a full commit message as per our usual practice.

Sorry about that Prof. I will go and figure out how to edit the commit message.

2023-02-09 15:37:06: 2023-02-09 15:37:06: [WIP] Adding MarkBind as an alternative docs option was created by @tlylt: 156

2023-02-09 15:37:09:
@CanIHasReview commented on 156

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-02-11 03:21:09:
@tlylt commented on 156

View details

Hi @damithc, a quick status update:

listed out the outline of the steps required, and some of the pending TODOs in the PR description
Will likely publish a release on the MarkBind side to address the blocker here, so that the rest can ideally be done by next week
have deployed a version of the adapted ab3 docs, mostly ready for a read-through/review of the presentation, at https://tlylt.github.io/ab3-docs-demo/index.html
for the 2103T side do you have any plan on how instructions need to be updated once this docs work is completed?

2023-02-11 12:37:46:
@Py0000 commented on 153

View details

I have updated the commit message.

2023-02-11 15:48:12:
@damithc commented on 156

View details

for the 2103T side do you have any plan on how instructions need to be updated once this docs work is completed?

For the time being (i.e., while this remains experimental), I can provide that information through the module website and se-edu/guides.

2023-02-14 04:17:04:
@tlylt commented on 156

View details

Thanks @damithc for the review, I have updated the docs accordingly.

can also preview it here https://tlylt.github.io/ab3-docs-demo/

Will also ask the rest of the MarkBind team to take a look to see if there are further edits needed.

2023-02-17 02:25:37: 2023-02-17 02:25:37: AY2223S2-CS2103T-W13-4 TutorPro was created by @Yufannnn: 157

2023-02-17 02:25:40:
@CanIHasReview commented on 157

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-02-28 10:52:35: 2023-02-28 10:52:35: Initialization was created by @roshankumar1991: 159

2023-03-06 15:06:53: 2023-03-06 15:06:53: About us update was created by @ShiJiaAo: 160

2023-03-06 15:06:56:
@CanIHasReview commented on 160

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-03-26 03:14:06:
@tlylt commented on 156

View details

Hi @damithc, I left some comments replying to your queries. Can let me know what you think when you have time? I think I'm still keen to follow up on this and finish it by this semester. Will also do a final upgrade of the markbind version when v5 is released.

2023-03-26 06:20:22:
@damithc commented on 156

View details

Sorry @tlylt , didn't realize you are waiting for my feedback:

A few other minor things:

Heading level mismatch?

UG and DG pdf version seems slightly shorter (i.e., fewer pages) in this version, which is good. The current AB3 uses orange color for headings. Should we do something similar in MarkBind version too (to make it look better, and also to show how to customize styles)?

And yes, get inputs from other MarkBind devs too.

For boxes, I'm wondering if the light or the seamless styles would make the content blend in with the main text more seamlessly. But this is a matter of taste and subjective.

In the end, we want to make the MarkBind version to look better (and editing not harder) than the current version or else there would be no incentive to switch.

2023-04-12 05:55:54: 2023-04-12 05:55:54: Bump nokogiri from 1.13.10 to 1.14.3 in /docs was created by @dependabot: 161

2023-04-12 05:55:58:
@CanIHasReview commented on 161

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-04-12 05:58:51:
@codecov-commenter commented on 161

View details

Codecov Report

Merging #161 (30c392b) into master (928f146) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##             master     #161   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-09 16:21:03:
@damithc commented on 138

View details

Putting this hold for now.

2023-05-09 16:26:04:
@damithc commented on 153

View details

@JsonProperty("tagged") List<JsonAdaptedTag> tagged) {

I think it is best if we modify both occurrences of tagged, if possible (or else we introduce an inconsistency which can be argued as even worse than the naming anomaly being fixed). So, I would like to explore that option before merging this PR.

2023-05-09 16:36:20:
@damithc commented on 152

View details

Sorry, it took a while for me to come back to this PR.
I'm wondering if including the target filename in the log message is better (rather than the current Prefs file not found. ..., Preferences file prefs.json not found. ...)

2023-05-11 13:55:28: 2023-05-11 13:55:28: [#147] Rename tagged to tags was created by @Eclipse-Dominator: 163

2023-05-11 13:55:31:
@CanIHasReview commented on 163

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:16:46: 2023-05-11 15:16:46: [#139] Rephrase 'file not found' logs shown during first run was created by @Eclipse-Dominator: 164

2023-05-11 15:16:49:
@CanIHasReview commented on 164

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:26:25:
@Eclipse-Dominator commented on 164

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

2023-05-11 15:52:06: 2023-05-11 15:52:06: [#73] no write access fail gracefully was created by @Eclipse-Dominator: 165

2023-05-11 15:52:10:
@CanIHasReview commented on 165

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 15:56:22:
@Eclipse-Dominator commented on 165

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

2023-05-11 16:14:42:
@Eclipse-Dominator edited comment on 165

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

I came up with the following solutions regarding the above

Immediately exit regardless of save errors
Adds a 3s timer to show the fail to exit before exiting (only on exit command)
Change warning message to tell the user to close via clicking on the x button [current approach]

I am not sure which solution is the right approach.

A separate issue is when the preference folder is not writeable. Error messages like so can still be seen

The IO exception is still caught here

        //Update config file in case it was missing to begin with or there are new/unused fields
        try {
            ConfigUtil.saveConfig(initializedConfig, configFilePathUsed);
        } catch (IOException e) {
            logger.warning("Failed to save config file : " + StringUtil.getDetails(e));
        }
        return initializedConfig;

So I am not sure if we should change this message.

2023-05-11 17:00:45: 2023-05-11 17:00:45: Rename tagged to tags was created by @Eclipse-Dominator: 168

2023-05-11 17:00:48:
@CanIHasReview commented on 168

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:15:09: 2023-05-11 17:15:09: [#147] rename tagged to tags was created by @Eclipse-Dominator: 169

2023-05-11 17:15:12:
@CanIHasReview commented on 169

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:15:27:
@CanIHasReview commented on 169

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v1 2/2] Change json property from tagged to tags

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-11 17:28:22: 2023-05-11 17:28:22: [#139] Rephrase 'file not found' during first run was created by @Eclipse-Dominator: 170

2023-05-11 17:28:26:
@CanIHasReview commented on 170

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 17:29:14:
@Eclipse-Dominator commented on 170

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I have made a separate issue regarding this: #167

2023-05-11 17:29:29:
@CanIHasReview commented on 170

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/4] Add logs for creation of default files
[v1 2/4] Change file not found log to file found
[v1 3/4] Modify warning to show file location
[v1 4/4] Adds more file paths to warnings

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-11 17:30:05:
@Eclipse-Dominator edited comment on 164

View details

While working on this, I realised that the

            initialData = new AddressBook();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + storage.getAddressBookFilePath()
                    + ". Will be starting with an empty AddressBook");
            initialData = new AddressBook();
        }

and

            initializedPrefs = new UserPrefs();
        } catch (IOException e) {
            logger.warning("Problem while reading from the file " + prefsFilePath
                    + ". Will be starting with an empty AddressBook");
            initializedPrefs = new UserPrefs();
        }

is unnecessary as the actual implementation of the methods used in the try block already handles the IO Exception, I will link a separate issue here #167

2023-05-11 17:31:06:
@Eclipse-Dominator commented on 164

View details

use #170

2023-05-11 20:04:43: 2023-05-11 20:04:43: [#73] no write access fail gracefully was created by @Eclipse-Dominator: 172

2023-05-11 20:04:47:
@CanIHasReview commented on 172

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-11 20:13:39:
@Eclipse-Dominator commented on 165

View details

Refer to #172

2023-05-11 23:23:43:
@CanIHasReview commented on 172

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/3] Improve error message if no write permission
[v1 2/3] Adds separate log for permission denied error
[v1 3/3] Adds back invalid command log

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-12 04:26:16:
@codecov-commenter commented on 169

View details

Codecov Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 09:56:44:
@CanIHasReview commented on 169

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v2 2/2] JsonAdaptedPerson: Use tags for JSON property (#169)

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 09:58:04:
@Eclipse-Dominator commented on 169

View details

I have updated the commit to include a commit body. I did do some testing with changes and no issues were spotted.

2023-05-12 10:29:45:
@CanIHasReview commented on 170

View details

v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 10:32:49:
@CanIHasReview commented on 170

View details

v3

@Eclipse-Dominator submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 11:02:00:
@CanIHasReview commented on 172

View details

v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-12 11:06:03:
@Eclipse-Dominator commented on 172

View details

Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

2023-05-12 11:06:10:
@Eclipse-Dominator edited comment on 172

View details

Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

2023-05-12 11:08:21:
@CanIHasReview commented on 170

View details

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/3] Add logs for creation of default files
[v4 2/3] Change file not found log to file found
[v4 3/3] MainApp: Modify warning to show file location

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/4/head:BRANCHNAME

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

2023-05-12 11:18:23: 2023-05-12 11:18:23: [#173] Null class logger not logging to logger output was created by @Eclipse-Dominator: 174

2023-05-12 11:18:26:
@CanIHasReview commented on 174

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-12 13:36:34:
@CanIHasReview commented on 169

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 13:36:36:
@CanIHasReview commented on 169

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] JsonAdaptedPerson class: Change `tagged` to `tags`
[v3 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-12 13:48:23:
@CanIHasReview commented on 170

View details

v5

@Eclipse-Dominator submitted v5 for review.

[v5 1/3] Add logs for creation of default files
[v5 2/3] Change file not found log to file found
[v5 3/3] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/5/head:BRANCHNAME

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

2023-05-12 13:51:18:
@Eclipse-Dominator commented on 169

View details

Yup I just realized it as well.

2023-05-12 13:54:45:
@CanIHasReview commented on 169

View details

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/2] JsonAdaptedPerson: Change variable name tagged -> tags
[v4 2/2] JsonAdaptedPerson: Change JSON property name tagged -> tags

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/169/4/head:BRANCHNAME

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

2023-05-12 13:58:43:
@codecov-commenter edited comment on 169

View details

Codecov Report

Merging #169 (6defd38) into master (f0c6861) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

Impacted Files	Coverage Δ
.../java/seedu/address/storage/JsonAdaptedPerson.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 14:08:28:
@damithc commented on 152

View details

Closing in favor of #170

2023-05-12 14:09:32:
@damithc commented on 169

View details

Thanks to @Py0000 for your contribution to this PR too.

2023-05-12 14:38:00:
@codecov-commenter commented on 174

View details

Codecov Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-12 17:41:57:
@CanIHasReview commented on 170

View details

v6

@Eclipse-Dominator submitted v6 for review.

[v6 1/2] Add logs for creation and discovery of default files
[v6 2/2] Modify logs to show target file location

(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/6/head:BRANCHNAME

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

2023-05-13 23:34:03:
@Eclipse-Dominator commented on 170

View details

I have modified and standardized some logs. Regarding the use of full stop, adding full stop to certain messagaes in the format like "TXT" + filename can feel rather messy, I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

2023-05-14 04:38:54:
@damithc commented on 170

View details

I have standardised a few log messages in the MainApp.java, I think subsequent standardisation should be done via a separate issue since this issue to rephrase file not found errors to be less error-like

@Eclipse-Dominator Yes, this PR should not touch other log messages. But the log messages touched in this PR should be locally consistent (i.e., consistent with the surrounding code).

2023-05-14 05:17:26:
@CanIHasReview commented on 170

View details

v7

@Eclipse-Dominator submitted v7 for review.

[v7 1/2] Make file-creation logs not resemble errors
[v7 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/7/head:BRANCHNAME

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

2023-05-14 05:47:03:
@damithc commented on 153

View details

Fixed by #169 that builds on this PR.
Thanks again for this PR @Py0000

2023-05-14 05:48:03:
@damithc commented on 77

View details

To be continued by #172
Thanks for this PR @ernestlim8

2023-05-14 09:05:54:
@CanIHasReview commented on 170

View details

v8

@Eclipse-Dominator submitted v8 for review.

[v8 1/2] Make file-creation logs not resemble errors
[v8 2/2] Show file location in log messages

(📚 Archive) (📈 Interdiff between v7 and v8) (📈 Range-Diff between v7 and v8)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/8/head:BRANCHNAME

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

2023-05-14 09:47:01:
@CanIHasReview commented on 170

View details

v9

@Eclipse-Dominator submitted v9 for review.

[v9 1/2] Make file-creation logs not resemble errors
[v9 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v8 and v9) (📈 Range-Diff between v8 and v9)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/9/head:BRANCHNAME

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

2023-05-14 15:00:15:
@CanIHasReview commented on 170

View details

v10

@Eclipse-Dominator submitted v10 for review.

[v10 1/2] Make file-creation logs not resemble errors
[v10 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v9 and v10) (📈 Range-Diff between v9 and v10)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/10/head:BRANCHNAME

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

2023-05-14 15:08:42: 2023-05-14 15:08:42: [#171] Updating gitignore to account for bin folder was created by @Eclipse-Dominator: 175

2023-05-14 15:08:44:
@CanIHasReview commented on 175

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-14 15:10:40:
@codecov-commenter commented on 175

View details

Codecov Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-14 15:10:58:
@CanIHasReview commented on 175

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add bin/ folder to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-14 15:11:36:
@codecov-commenter edited comment on 175

View details

Codecov Report

Merging #175 (17dc864) into master (1e05fef) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #175   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-14 15:38:37:
@damithc commented on 170

View details

@se-edu/tech-team-level1 for your review, if any of you are available these days.
Please review each commit independently, including the commit message. Ensure the diff matches the commit message. Don't hesitate to nitpick, as we want to polish up each commit as much as we can, before merging.

2023-05-14 16:12:24:
@damithc commented on 175

View details

I could not reproduce this behavior on Windows i.e., I ran gradlew but it did not create a bin folder. 🤔

Commit message: generated -> generates?

2023-05-15 07:19:32:
@damithc commented on 172

View details

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

hmm... there is no real need to save data upon exit. Perhaps we can skip the data saving if the command is the exit command?

2023-05-15 07:49:25:
@Eclipse-Dominator commented on 175

View details

After some investigation, the creation of bin/ folder seems to be caused by a separate vscode extension. As the focus of the module is the use of Intellij, let's close this for now.

2023-05-15 08:10:35:
@CanIHasReview commented on 170

View details

v11

@Eclipse-Dominator submitted v11 for review.

[v11 1/2] Make file-creation logs not resemble errors
[v11 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/11/head:BRANCHNAME

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

2023-05-16 03:07:39:
@Eclipse-Dominator commented on 172

View details

Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

2023-05-16 03:10:21:
@Eclipse-Dominator edited comment on 172

View details

Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks.

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

2023-05-16 04:24:50:
@codecov-commenter commented on 172

View details

Codecov Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-16 04:53:46:
@damithc commented on 172

View details

As this is a very rare case, we should try a solution that minimizes changes to the code. So, approach 3 should be fine.
Do you think this PR can be just one commit?

2023-05-16 05:27:24:
@Eclipse-Dominator commented on 172

View details

I think 2 commit is probably more suited here?

Decoupling IOException and AccessDeniedException
Decoupling ParseException and CommandException

As for the commit body, I am thinking of

Better error message when file have no write-access

When the data file has no write access, a message containing.
a raw exception is displayed to the user with no specific instructions
on handling such errors.

Let's 
* Polish up error messages displayed to the user
* Decouple IOExceptions and AccessDeniedExceptions
* Add instructions to ask the users to manually close AB3

Decoupling of ParseException and CommandException log messages

All errors related to command is logged as "invalid command".
Let's be more specific by decoupling ParseExceptions and CommandExceptions
to enable an more accurate logging of the errors.

2023-05-16 05:54:22:
@damithc commented on 172

View details

I'm not sure if the second commit adds enough value. While the same prefix is used in both cases, the error message itself should tell us what exactly happened, right?
Don't forget that commit message subject should be in imperative mood.

2023-05-17 05:25:22:
@CanIHasReview commented on 170

View details

v12

@Eclipse-Dominator submitted v12 for review.

[v12 1/2] Make file-creation logs not resemble errors
[v12 2/2] Make file-related log messages more informative

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/170/12/head:BRANCHNAME

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

2023-05-17 05:27:03:
@Eclipse-Dominator commented on 170

View details

Updated iteration fixing the line breaks, please update if there is any more changes required @se-edu/tech-team-level1

2023-05-17 21:02:34: 2023-05-17 21:02:34: [#162] Show warning when ignoring repeated parameters was created by @Eclipse-Dominator: 176

2023-05-17 21:02:37:
@CanIHasReview commented on 176

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-17 21:03:09:
@CanIHasReview commented on 176

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Show warning when ignoring repeated parameters

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-17 21:09:11:
@codecov-commenter commented on 176

View details

Codecov Report

Merging #176 (ce23ddf) into master (1e05fef) will decrease coverage by 0.23%.
The diff coverage is 58.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
- Coverage     72.15%   71.93%   -0.23%     
- Complexity      399      402       +3     
============================================
  Files            70       70              
  Lines          1232     1247      +15     
  Branches        125      131       +6     
============================================
+ Hits            889      897       +8     
- Misses          311      312       +1     
- Partials         32       38       +6

Impacted Files	Coverage Δ
.../seedu/address/logic/parser/EditCommandParser.java	`81.25% <33.33%> (-11.06%)`	⬇️
...java/seedu/address/logic/commands/EditCommand.java	`93.42% <72.72%> (-3.60%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 05:36:18: 2023-05-18 05:36:18: [#166] Bump javafx to version 11.0.2 was created by @Eclipse-Dominator: 177

2023-05-18 05:36:21:
@CanIHasReview commented on 177

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-18 05:38:48:
@CanIHasReview commented on 177

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/2] Fix JavaFx compatibility issue
[v1 2/2] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 09:18:37:
@Eclipse-Dominator commented on 176

View details

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

2023-05-18 09:52:48:
@damithc commented on 176

View details

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

Yup, this may still be the best option but it's better to explore other options before making a decision.

2023-05-18 11:44:23: 2023-05-18 11:44:23: [#178] Include JVM Crash Logs in gitignore was created by @Eclipse-Dominator: 179

2023-05-18 11:44:26:
@CanIHasReview commented on 179

View details

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

2023-05-18 11:46:22:
@CanIHasReview commented on 179

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add hs_err_pid log files to .gitignore

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 11:47:14:
@codecov-commenter commented on 179

View details

Codecov Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:48:55:
@codecov-commenter edited comment on 179

View details

Codecov Report

Merging #179 (6eb0d19) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:53:23:
@CanIHasReview commented on 177

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Fix JavaFx compatibility issue

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-18 11:55:09:
@codecov-commenter commented on 177

View details

Codecov Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 11:56:36:
@codecov-commenter edited comment on 177

View details

Codecov Report

Merging #177 (a6fc3c0) into master (a3f7c82) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1236     1236           
  Branches        127      127           
=========================================
  Hits            889      889           
  Misses          315      315           
  Partials         32       32

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-18 12:04:32:
@CanIHasReview commented on 174

View details

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Add assertion for null class loggers

(📚 Archive)

Checkout this PR version locally

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

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

2023-05-18 13:34:52:
@damithc commented on 179

View details

Thanks for the PR @Eclipse-Dominator
I felt the change is simple enough to warrant a subject-only commit message, so I took the liberty of shortening it.

2023-05-18 13:53:39:
@damithc commented on 177

View details

@zhongfu I remember you did some investigation into this problem earlier. If you are free, your inputs are welcome.

2023-05-19 12:01:04:
@Eclipse-Dominator commented on 172

View details

I have updated the commit to the following:

Improve error message if no write permission

Insufficient permission to read the data file will show
an AccessDeniedException as error message.
AB3 should be able to fail gracefully with instructions
to close the application upon failure.

Let's
- make the error message more user friendly by informing
the user that there are insufficient permissions for the file.
- Decouple AccessDeniedException from IOException messages.
- Decouple ParseException and CommandException log messages.

2023-05-19 12:03:23:
@CanIHasReview commented on 174

View details

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Add assertion for null class loggers

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

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

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

2023-05-20 00:50:09:
@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz with class in the commit since clazz is a term to replace class in java to avoid using preserved keyword.

2023-05-20 00:50:27:
@CanIHasReview commented on 174

View details

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Add a null check to LogCenter.getLogger(Class)

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

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

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

2023-05-20 00:51:24:
@codecov edited comment on 174

View details

Codecov Report

Merging #174 (c715bfa) into master (b581276) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #174      +/-   ##
============================================
+ Coverage     71.92%   72.06%   +0.13%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1236     1235       -1     
  Branches        127      127              
============================================
+ Hits            889      890       +1     
+ Misses          315      314       -1     
+ Partials         32       31       -1

Impacted Files	Coverage Δ
...in/java/seedu/address/commons/core/LogsCenter.java	`83.33% <100.00%> (+4.95%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

2023-05-20 04:23:08:
@codecov edited comment on 172

View details

Codecov Report

Merging #172 (ea502d3) into master (f0c6861) will decrease coverage by 0.35%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
- Coverage     72.15%   71.80%   -0.35%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1232     1238       +6     
  Branches        125      127       +2     
============================================
  Hits            889      889              
- Misses          311      317       +6     
  Partials         32       32

Impacted Files	Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java	`0.00% <0.00%> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`68.18% <33.33%> (-6.82%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Eclipse-Dominator · 2023-05-29T03:43:08Z

Digest 2023-05-22T07:34:19Z

Tracked {} changes in issues

dummy test 1234 #2

XXX edited

lorem ipsyum

asdf...

XY commented

lorem ipsyum

asdf...

dummy test 1234 #3

XXX created issue

"content
asdf ...

Eclipse-Dominator · 2023-05-29T05:08:10Z

Digest 2023-05-29T05:08:15Z

Tracked 0 changes across 0 issues

Eclipse-Dominator · 2023-05-29T05:14:28Z

Digest 2023-05-29T05:14:33Z

Tracked 12 changes across 3 issues

gen #3

created by Eclipse-Dominator on 2023-05-23 15:27:39
lorem ipsum

gen #2

created by Eclipse-Dominator on 2023-05-23 15:27:34
lorem ipsum

test #1

modified by Eclipse-Dominator on 2023-05-23 02:02:39
DUMMY ASDASDasdfadsf

Eclipse-Dominator commented on 2023-05-22 07:02:45
2023-05-09 16:21:03:

    <details>
    <summary>damithc commented on [138](https://github.com/se-edu/addressbook-level3/pull/138)</summary>

    >Putting this hold for now.

    </det...

Eclipse-Dominator modified comment on 2023-05-22 07:07:58
2023-05-09 16:21:03:

@damithc commented on 138

View details

Putting this hold for now.

...

Eclipse-Dominator commented on 2023-05-22 07:14:56
2023-05-20 00:50:09:

@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz ...

Eclipse-Dominator commented on 2023-05-22 07:17:21
2023-05-20 00:50:09:

@Eclipse-Dominator commented on 174

View details

I have replaced certain instance of clazz ...

Eclipse-Dominator commented on 2023-05-22 07:22:54

>

view digest

> > 2023-05-20 00:50:09: > @Eclipse-Dominator commented on [174](https://github.com/se-edu/addressbook-level3/pull/174) >

>

View details

> ...

Eclipse-Dominator commented on 2023-05-22 07:24:34

>

Digest 2023-05-22T07:24:35Z

> > 2023-05-20 00:50:09: > @Eclipse-Dominator commented on [174](https://github.com/se-edu/addressbook-level3/pull/174) >

> Eclipse-Dominator commented on 2023-05-22 07:25:44

>

Digest 2023-05-22T07:25:45Z

> > 2023-05-20 00:50:09: > @Eclipse-Dominator commented on [174](https://github.com/se-edu/addressbook-level3/pull/174) >

> Eclipse-Dominator commented on 2023-05-22 07:25:46

>

Digest 2023-05-22T07:25:47Z

> > >

Eclipse-Dominator commented on 2023-05-22 07:34:18

>

Digest 2023-05-22T07:34:19Z

> > 2023-01-27 16:26:11: > @damithc commented on [153](https://github.com/se-edu/addressbook-level3/pull/153) >

>

View det...

Eclipse-Dominator modified comment on 2023-05-23 02:17:37
test1234

Eclipse-Dominator modified comment on 2023-05-29 04:20:30

>

Digest 2023-05-22T07:34:19Z

>

Tracked {} changes in issues

>

> > # dummy test 1234 [#2](https://github.com/Eclipse-Dominator/Github_digest/issues/2) > XXX ...

Eclipse-Dominator commented on 2023-05-29 05:08:10

>

Digest 2023-05-29T05:08:15Z

>

Tracked 0 changes across 0 issues

> > > >

Eclipse-Dominator added the bug Something isn't working label May 23, 2023

This was referenced May 29, 2023

Issues Digest #7

Open

Issues Digest #8

Open

Issues Digest #9

Open

Issues Digest #14

Open

Eclipse-Dominator mentioned this issue Jun 7, 2023

Issues Digest #49

Open

test #1

test #1

Comments

Eclipse-Dominator commented May 22, 2023 • edited Loading

Eclipse-Dominator commented May 22, 2023 • edited Loading

Eclipse-Dominator commented May 22, 2023

Eclipse-Dominator commented May 22, 2023 • edited Loading

v1

v1

v1

Codecov Report

v2

v2

v3

v2

v4

v3

v3

v5

v4

Codecov Report

Codecov Report

v6

v7

v8

v9

v10

Codecov Report

v1

Codecov Report

v11

Codecov Report

v12

v1

Codecov Report

v1

v1

Codecov Report

Codecov Report

v2

Codecov Report

Codecov Report

v1

v2

v3

Codecov Report

Codecov Report

Eclipse-Dominator commented May 22, 2023

v3

Eclipse-Dominator commented May 22, 2023

v3

Eclipse-Dominator commented May 22, 2023

v3

Eclipse-Dominator commented May 22, 2023

Digest 2023-05-22T07:24:35Z

v3

Eclipse-Dominator commented May 22, 2023

Digest 2023-05-22T07:25:45Z

v3

Eclipse-Dominator commented May 22, 2023

Digest 2023-05-22T07:25:47Z

Eclipse-Dominator commented May 22, 2023

Digest 2023-05-22T07:34:19Z

Codecov Report

Codecov Report

v1

v1

v1

Codecov Report

v2

v2

v3

v2

v4

v3

v3

v5

v4

Codecov Report

Codecov Report

Eclipse-Dominator commented May 22, 2023 •

edited

Loading

Eclipse-Dominator commented May 22, 2023 •

edited

Loading

Eclipse-Dominator commented May 22, 2023 •

edited

Loading

Eclipse-Dominator commented May 29, 2023 •

edited

Loading