PR 2 for v1.15.0 (Changes Related to New Connector Development, Upstream Starter Project Changes, Etc.) #61
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexjhawk
force-pushed
the
dev/alha-pr2
branch
from
59234a6
to
c760be8
Compare
src/main/java/com/hms_networks/americas/sc/extensions/json/JSONObject.java
Show resolved
Hide resolved
it-hms
requested changes
Sep 27, 2023
| @@ -311,6 +311,34 @@ public static void LOG_DEBUG(String logString) { | |||
| LOG(LOG_LEVEL_DEBUG, logString); | |||
| } | |||
|
|
|||
| /** | |||
| * Log a string and exception with the debug log level | |||
There was a problem hiding this comment.
Perhaps is top-level comment is a sentence, it should end in period. Not sure if this looks better or worse for doc strings.
For params and returns, this is not necessary.
There was a problem hiding this comment.
Added punctuation. I initially did not include these because it maintained consistency with the class, but I have added an additional commit to address this.
There was a problem hiding this comment.
In the future, what do you think about documenting consistency concerns like this and then circling back after the PR?
- It makes sense to address the punctuation directly added/modified/lack thereof in the pull request, but subsequent concerns about consistency could be more effectively addressed.
- In the past, we've mostly addressed the consistency concern(s) in the PR, but this also introduces scope creep to PRs, making them larger and more cumbersome to review and modify.
- Scope creep in PRs can also increase the time required to work with a PR because it can introduce unexpected modification/rebasing of content that was otherwise edited in other commits and could conflict with other staged commits in certain scenarios, depending on order.
| * @param logString string to log | ||
| * @param logException exception to log | ||
| * @param elevateExceptionLogLevel elevates the log level used for the specified exception to | ||
| * debug |
| * @param logString string to log | ||
| * @param logException exception to log | ||
| * @param elevateExceptionLogLevel elevates the log level used for the specified exception to | ||
| * debug |
| * @param logString string to log | ||
| * @param logException exception to log | ||
| * @param elevateExceptionLogLevel elevates the log level used for the specified exception to | ||
| * debug |
| * @param logString string to log | ||
| * @param logException exception to log | ||
| * @param elevateExceptionLogLevel elevates the log level used for the specified exception to | ||
| * debug |
There was a problem hiding this comment.
Will make these changes shortly.
TomKimsey
reviewed
Sep 28, 2023
alexjhawk
force-pushed
the
dev/alha-pr2
branch
from
c760be8
to
12f92bc
Compare
Improved the exception logging capability of the Logger.java class to allow for exceptions at other log levels. This is facilitating the upcoming changes to allow combined exception logging with a standard log string.
Added combined logging methods in Logger.java which allow for a log string to be passed alongside an exception. There is the ability to both output an exception at the same log level as the associated string, or at the exception/trace log level.
Added punctuation to existing Javadocs in the Logger.java class to ensure consistency with the expected format.
Added non-null put methods to the JSONObject class which can significantly improve the code structure in certain scenarios. Many instances of building JSONObjects simply use constants passed for the key, and a number of passed in values frequently are already validated (whether from another request, or a type-constrained source). These additional methods allow for cleaner source code in scenarios where we do not expect to ever encounter a null or invalid key/value. Any exceptions resulting from the usage of these methods would indicate that the method is either being used improperly/shouldn't be used at all.
Fixed an improperly formatted Javadoc return description in JSONObject.java.
This project previously did not have a jvmrun file, but in an effort to better keep the starter project and child project in sync, it is being added.
alexjhawk
force-pushed
the
dev/alha-pr2
branch
from
12f92bc
to
aaeb506
Compare
it-hms
approved these changes
Sep 28, 2023
TomKimsey
approved these changes
Sep 28, 2023
oliver-walker-hms
approved these changes
Sep 28, 2023
|
I expect to have PR 3 ready either later today or tomorrow. It should be the final PR and will mainly consist of web docs (how to use the library only, no detailed documentation...yet.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is PR 2 in a series of pull requests to bring in changes for v1.15.0. These are mostly related to the development of a new connector, staged changes that were previously untested, and other changes or enhancements, as described.