Add lower level logging statement #203

Eclipse-Dominator · 2023-07-28T01:11:24Z

Fixes #194

Includes some lower-level logging statements as well as moving some of the existing logging statements to other levels.

canihasreview · 2023-07-28T01:11:27Z

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

See this repository's contribution guide for more information.

codecov · 2023-07-28T01:13:17Z

Codecov Report

Merging #203 (7c506ec) into master (08fe6e3) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
- Coverage     75.26%   75.26%   -0.01%     
  Complexity      419      419              
============================================
  Files            71       71              
  Lines          1334     1338       +4     
  Branches        126      126              
============================================
+ Hits           1004     1007       +3     
- Misses          300      301       +1     
  Partials         30       30

Files Changed	Coverage Δ
src/main/java/seedu/address/MainApp.java	`0.00% <0.00%> (ø)`
.../seedu/address/logic/parser/AddressBookParser.java	`100.00% <100.00%> (ø)`

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

canihasreview · 2023-07-28T02:00:54Z

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/3] Exempt constructors from Javadoc requirement
[v1 2/3] Enhance logging with lower-level statements
[v1 3/3] Adjust logger level change for consistent log output

(📚 Archive)

Checkout this PR version locally

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

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

damithc

Some suggestions added. Mainly, to reduce the blast radius of this code update. As it is a low-gain change, let's keep the cost low.

damithc · 2023-07-29T12:15:42Z

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

@@ -53,14 +53,13 @@ public void init() throws Exception {



Suggested commit message:

In the MainApp class, let's move the call to the initLogging method closer to the start of the execution start point, to minimize the possibility of log statements being executed before the desired log level is set.

I missed this earlier. Adjust the commit message subject to match the body.

config/checkstyle/checkstyle.xml

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

src/main/java/seedu/address/logic/parser/AddressBookParser.java

damithc · 2023-07-29T12:40:12Z

src/main/java/seedu/address/logic/parser/AddressBookParser.java

            return new HelpCommand();

        default:
+            logger.fine("Unknown command detected");


Suggested change

logger.fine("Unknown command detected");

logger.finer("This user input caused a ParseException: " + userInput);

src/main/java/seedu/address/logic/parser/AddressBookParser.java

damithc · 2023-07-29T12:49:41Z

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

@@ -74,6 +74,8 @@ public void init() throws Exception {
     * or an empty address book will be used instead if errors occur when reading {@code storage}'s address book.


Suggested commit message:

Add examples of lower-level log messages Let's add a few examples of lower-level log messages, to illustrate to developers how they can be used in this codebase.

canihasreview · 2023-07-29T13:58:10Z

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/2] Add examples of lower-level log messages
[v2 2/2] Adjust logger level change for consistent log output

(📚 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/203/2/head:BRANCHNAME

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

damithc

Looks like my review was stuck in pending state until now.

damithc · 2023-07-29T14:14:48Z

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

@@ -142,7 +143,7 @@ protected Config initConfig(Path configFilePath) {
     */
    protected UserPrefs initPrefs(UserPrefsStorage storage) {
        Path prefsFilePath = storage.getUserPrefsFilePath();
-        logger.info("Using preference file : " + prefsFilePath);
+        logger.fine("Using preference file : " + prefsFilePath);

        UserPrefs initializedPrefs;


Use fine/finer for info that users shouldn't see but useful for developers. If the log messages in this class are useful for users, use info level- otherwise OK to use the fine level.

My opinion on this is that users should only see logs when a new file is created or some error occurred while trying to load the file. When the file is loaded correctly from the path, I feel that the logs here aren't actually useful for the users but more for developers since it's expected to be loaded correctly for the user which was why I opt to use fine for logs that say "Using xxx file: ".

The argument to use info logs might be if the user wants to edit/modify the related files manually so it might be useful to show file path location with info but based on the ug, so there isn't really a need to display the file locations.

That is a fair point. But the UG mentions the data file only.
Also, the log is not for errors/warnings only -- if we wanted that, we should have set the default log level to WARN or ERROR. At INFO level, it should show important activities that are happening.

damithc · 2023-07-29T14:15:59Z

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

@@ -53,14 +53,13 @@ public void init() throws Exception {



I missed this earlier. Adjust the commit message subject to match the body.

src/main/java/seedu/address/logic/parser/AddressBookParser.java

canihasreview · 2023-08-07T22:33:08Z

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] Add examples of lower-level log messages
[v3 2/2] Adjust log initialization position in MainApp

(📚 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/203/3/head:BRANCHNAME

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

damithc · 2023-08-09T08:31:19Z

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

damithc · 2023-08-09T08:32:31Z

@Eclipse-Dominator pls update the PR description to match the current state of the changes done.

markusyeo · 2023-08-09T08:44:03Z

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

@@ -74,6 +74,8 @@ public void init() throws Exception {
     * or an empty address book will be used instead if errors occur when reading {@code storage}'s address book.
     */
    private Model initModelManager(Storage storage, ReadOnlyUserPrefs userPrefs) {
+        logger.info("Using data file : " + storage.getAddressBookFilePath());


There looks like there is an extra space here before the colon

Current code:
logger.info("Using data file : " + storage.getAddressBookFilePath());

Proposed Change:
logger.info("Using data file: " + storage.getAddressBookFilePath());

Hi sorry for the late update, the formatting is like to follow the other similar logging statements found in MainApp where a space is included before the colon.

hansstanley · 2023-08-09T12:09:40Z

src/main/java/seedu/address/logic/parser/AddressBookParser.java

+        // Currently, lower level log messages are used sparingly, to minimize noise in the code.
+        logger.fine("Command word: " + commandWord + " ; Arguments: " + arguments);


Perhaps this comment can be rephrased so it doesn't describe a point in time, as markers like "currently" make code comments harder to maintain.

Instead, it could describe the desired usage, for e.g. "Lower level log messages should be used sparingly to minimise noise in the code."

Also there's a space before the inline semicolon, not sure if that's intended.

Suggested change

// Currently, lower level log messages are used sparingly, to minimize noise in the code.

logger.fine("Command word: " + commandWord + " ; Arguments: " + arguments);

// Lower level log messages should be used sparingly to minimize noise in the code.

logger.fine("Command word: " + commandWord + "; Arguments: " + arguments);

Instead, it could describe the desired usage, for e.g. "Lower level log messages should be used sparingly to minimise noise in the code."

Good point @hansstanley

SPWwj

LGTM. Just some minor suggestion to make the comments more succinct.

SPWwj · 2023-08-09T13:06:04Z

src/main/java/seedu/address/logic/parser/AddressBookParser.java

@@ -69,6 +78,7 @@ public Command parseCommand(String userInput) throws ParseException {
            return new HelpCommand();

        default:
+            logger.finer("This user input caused a ParseException: " + userInput);


This is redundant, as you have already specified the userInput.

Suggested change

logger.finer("This user input caused a ParseException: " + userInput);

logger.finer("User input caused a ParseException: " + userInput);

Or

Suggested change

logger.finer("This user input caused a ParseException: " + userInput);

logger.finer("ParseException: " + userInput);

SPWwj · 2023-08-09T13:23:25Z

src/main/java/seedu/address/logic/parser/AddressBookParser.java

@@ -42,6 +45,12 @@ public Command parseCommand(String userInput) throws ParseException {

        final String commandWord = matcher.group("commandWord");
        final String arguments = matcher.group("arguments");
+
+        // Note to developers: Change the log level in config.json to enable lower level (i.e., FINE and lower)


Suggested change

// Note to developers: Change the log level in config.json to enable lower level (i.e., FINE and lower)

// Note to developers: Change the log level in config.json to enable lower level (i.e., FINE, FINER and lower)

damithc · 2023-08-14T13:20:27Z

@Eclipse-Dominator reminder about this

Let's add a few examples of lower-level log messages, to illustrate to developers how they can be used in this codebase.

In the MainApp class, let's move the call to the initLogging method closer to the start of the execution start point, to minimize the possibility of log statements being executed before the desired log level is set.

canihasreview · 2023-08-15T04:19:30Z

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/2] Add examples of lower-level log messages
[v4 2/2] Adjust log initialization position in MainApp

(📚 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/203/4/head:BRANCHNAME

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

damithc · 2023-08-20T08:41:52Z

src/main/java/seedu/address/logic/parser/AddressBookParser.java

@@ -69,6 +78,7 @@ public Command parseCommand(String userInput) throws ParseException {
            return new HelpCommand();

        default:
+            logger.finer("ParseException: " + userInput);


Suppose the user input blah caused a ParseException. Compare the following two log messages:

ParseException: blah
This user input cased a ParseException: blah

I think the latter is more self-explanatory. The former can even mislead the reader to think blah is the ParseException.
What do you think?

damithc · 2023-08-25T14:31:17Z

@Eclipse-Dominator shall we wrap this up soon?

damithc · 2023-09-03T11:57:51Z

Added these commits to master with minor tweaks.

Eclipse-Dominator force-pushed the add-lower-level-logging-statement branch 4 times, most recently from 769e15f to 7a4ad45 Compare July 28, 2023 01:56

canihasreview bot added the s.ToReview label Jul 28, 2023

damithc requested changes Jul 29, 2023

View reviewed changes

Eclipse-Dominator force-pushed the add-lower-level-logging-statement branch 2 times, most recently from 0304e27 to 141e6cf Compare July 29, 2023 13:56

canihasreview bot requested a review from damithc July 29, 2023 13:58

damithc requested changes Aug 4, 2023

View reviewed changes

Eclipse-Dominator force-pushed the add-lower-level-logging-statement branch 3 times, most recently from 0486316 to 7b5e6f4 Compare August 7, 2023 22:32

canihasreview bot requested a review from damithc August 7, 2023 22:33

markusyeo reviewed Aug 9, 2023

View reviewed changes

hansstanley reviewed Aug 9, 2023

View reviewed changes

SPWwj reviewed Aug 9, 2023

View reviewed changes

Eclipse-Dominator force-pushed the add-lower-level-logging-statement branch from 7b5e6f4 to 2d172b9 Compare August 15, 2023 04:17

Eclipse-Dominator added 2 commits August 15, 2023 12:17

Add examples of lower-level log messages

acb0b8c

Let's add a few examples of lower-level log messages, to illustrate to developers how they can be used in this codebase.

Adjust log initialization position in MainApp

7c506ec

In the MainApp class, let's move the call to the initLogging method closer to the start of the execution start point, to minimize the possibility of log statements being executed before the desired log level is set.

Eclipse-Dominator force-pushed the add-lower-level-logging-statement branch from 2d172b9 to 7c506ec Compare August 15, 2023 04:17

damithc requested changes Aug 20, 2023

View reviewed changes

damithc closed this Sep 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lower level logging statement #203

Add lower level logging statement #203

Eclipse-Dominator commented Jul 28, 2023 •

edited

Loading

canihasreview bot commented Jul 28, 2023

codecov bot commented Jul 28, 2023 •

edited

Loading

canihasreview bot commented Jul 28, 2023

damithc left a comment

damithc Jul 29, 2023

damithc Jul 29, 2023

damithc Jul 29, 2023

damithc Jul 29, 2023

canihasreview bot commented Jul 29, 2023

damithc left a comment

damithc Jul 29, 2023

Eclipse-Dominator Aug 7, 2023 •

edited

Loading

damithc Aug 7, 2023

damithc Jul 29, 2023

canihasreview bot commented Aug 7, 2023

damithc commented Aug 9, 2023

damithc commented Aug 9, 2023

markusyeo Aug 9, 2023

Eclipse-Dominator Aug 15, 2023

hansstanley Aug 9, 2023 •

edited

Loading

damithc Aug 9, 2023

SPWwj left a comment

SPWwj Aug 9, 2023

SPWwj Aug 9, 2023

damithc commented Aug 14, 2023

canihasreview bot commented Aug 15, 2023

damithc Aug 20, 2023

damithc commented Aug 25, 2023

damithc commented Sep 3, 2023

	logger.fine("Unknown command detected");
	logger.finer("This user input caused a ParseException: " + userInput);

		@@ -74,6 +74,8 @@ public void init() throws Exception {
		* or an empty address book will be used instead if errors occur when reading {@code storage}'s address book.

		// Currently, lower level log messages are used sparingly, to minimize noise in the code.
		logger.fine("Command word: " + commandWord + " ; Arguments: " + arguments);

	logger.finer("This user input caused a ParseException: " + userInput);
	logger.finer("User input caused a ParseException: " + userInput);

	logger.finer("This user input caused a ParseException: " + userInput);
	logger.finer("ParseException: " + userInput);

	// Note to developers: Change the log level in config.json to enable lower level (i.e., FINE and lower)
	// Note to developers: Change the log level in config.json to enable lower level (i.e., FINE, FINER and lower)

Add lower level logging statement #203

Add lower level logging statement #203

Conversation

Eclipse-Dominator commented Jul 28, 2023 • edited Loading

canihasreview bot commented Jul 28, 2023

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

canihasreview bot commented Jul 28, 2023

v1

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 29, 2023

v2

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eclipse-Dominator Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Aug 7, 2023

v3

damithc commented Aug 9, 2023

damithc commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansstanley Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SPWwj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Aug 14, 2023

canihasreview bot commented Aug 15, 2023

v4

Choose a reason for hiding this comment

damithc commented Aug 25, 2023

damithc commented Sep 3, 2023

Eclipse-Dominator commented Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

Eclipse-Dominator Aug 7, 2023 •

edited

Loading

hansstanley Aug 9, 2023 •

edited

Loading