Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add basic logger (#20) #27

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Conversation

yokuze
Copy link
Contributor

@yokuze yokuze commented Mar 5, 2019

Note: both request / access logs: #28 and sampling have been broken out into a separate ticket: #26 to be addressed in a different PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 99.196% when pulling ecdccc9 on yokuze:logging_20 into 250c469 on silvermine:master.

@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage increased (+0.06%) to 99.265% when pulling 2dca77e on yokuze:logging_20 into a7b2af1 on silvermine:master.

trace(msg: string, data: unknown): void;

/**
* Log a message at the `debug` log level. The logger will only log a message for this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bunch more of these in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

*/
protected _shouldLog(level: LogLevel): boolean {
// Log if the level is higher priority than the current log level setting.
// e.g. error (50) >= info (30)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some double spaces here

return levels[level] <= levels.debug;
}

export default isDebugLevelOrMoreVerbose;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why export default wasn't just added to the function declaration above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can think of.

@jthomerson
Copy link
Member

Pushed these changes (ignoring all space since there were a lot of whitespace fixes):

jrthomer@lappy ~/git-clones/silvermine/lambda-express
[ logging_20 ] $ git diff --ignore-all-space ecdccc9..3bf98ba
diff --git a/src/logging/is-debug-level-or-more-verbose.ts b/src/logging/is-debug-level-or-more-verbose.ts
index 430da74..9c0c20e 100644
--- a/src/logging/is-debug-level-or-more-verbose.ts
+++ b/src/logging/is-debug-level-or-more-verbose.ts
@@ -5,9 +5,7 @@ import levels from './levels';
  * @returns `true` if the given log level is `'debug'` or a more verbose level (e.g.
  * `'trace'`).
  */
-function isDebugLevelOrMoreVerbose(level: LogLevel): boolean {
+export default function isDebugLevelOrMoreVerbose(level: LogLevel): boolean {
    // More verbose levels have lower priority numbers
    return levels[level] <= levels.debug;
 }
-
-export default isDebugLevelOrMoreVerbose;
diff --git a/src/logging/logging-types.ts b/src/logging/logging-types.ts
index d52cbc8..e094b67 100644
--- a/src/logging/logging-types.ts
+++ b/src/logging/logging-types.ts
@@ -127,7 +127,7 @@ export type LogLevels = {
 };
 
 /**
- * Configuration to initialize a Logger with.
+ * Configuration to initialize a `Logger` with.
  */
 export interface LoggerConfig {
 
@@ -193,7 +193,7 @@ export interface DebugLogObject extends LogObject {
    remaining: number;
 
    /**
-    * The Lambda event source type that triggered the current Request.
+    * The Lambda event source type that triggered the current `Request`.
     */
    int: LambdaEventSourceType;

@yokuze
Copy link
Contributor Author

yokuze commented Mar 25, 2019

@jthomerson Here's the diff of the actual changes:

diff --git a/src/Router.ts b/src/Router.ts
index c6c1619..bd32fc8 100644
--- a/src/Router.ts
+++ b/src/Router.ts
@@ -16,6 +16,7 @@ import { RouteMatchingProcessorChain } from './chains/RouteMatchingProcessorChai
 import { MatchAllRequestsProcessorChain } from './chains/MatchAllRequestsProcessorChain';
 import { SubRouterProcessorChain } from './chains/SubRouterProcessorChain';
 import Route from './Route';
+import { Optional } from './utils/common-types';

 const DEFAULT_OPTS: RouterOptions = {
    caseSensitive: false,
@@ -27,7 +28,7 @@ export default class Router implements IRouter {
    public readonly routerOptions: RouterOptions;
    private readonly _processors: IRequestMatchingProcessorChain[] = [];

-   public constructor(options?: RouterOptions) {
+   public constructor(options?: Optional<RouterOptions>) {
       this.routerOptions = _.defaults(options, DEFAULT_OPTS);
    }

diff --git a/src/utils/common-types.ts b/src/utils/common-types.ts
index 4f9c984..ed95855 100644
--- a/src/utils/common-types.ts
+++ b/src/utils/common-types.ts
@@ -6,6 +6,8 @@ export interface StringArrayOfStringsMap { [s: string]: string[] }
 export interface KeyValueStringObject { [k: string]: (string | string[] | KeyValueStringObject) }
 // Removes `readonly` modifier and make all keys writable again
 export type Writable<T> = { -readonly [P in keyof T]-?: T[P] };
+// Makes all keys on an object optional
+export type Optional<T> = { [P in keyof T]? : T[P] };

@yokuze
Copy link
Contributor Author

yokuze commented Mar 25, 2019

@jthomerson if you accept the approach above, I'll open a ticket / PR to add Optional<T> to our toolbox project.

@jthomerson
Copy link
Member

I just pushed a change to use Partial instead of our own Optional, ADIRL

jrthomer@lappy ~/git-clones/silvermine/lambda-express
[ logging_20 ] $ git diff
diff --git a/src/Router.ts b/src/Router.ts
index bd32fc8..8bf9848 100644
--- a/src/Router.ts
+++ b/src/Router.ts
@@ -16,7 +16,6 @@ import { RouteMatchingProcessorChain } from './chains/RouteMatchingProcessorChai
 import { MatchAllRequestsProcessorChain } from './chains/MatchAllRequestsProcessorChain';
 import { SubRouterProcessorChain } from './chains/SubRouterProcessorChain';
 import Route from './Route';
-import { Optional } from './utils/common-types';
 
 const DEFAULT_OPTS: RouterOptions = {
    caseSensitive: false,
@@ -28,7 +27,7 @@ export default class Router implements IRouter {
    public readonly routerOptions: RouterOptions;
    private readonly _processors: IRequestMatchingProcessorChain[] = [];
 
-   public constructor(options?: Optional<RouterOptions>) {
+   public constructor(options?: Partial<RouterOptions>) {
       this.routerOptions = _.defaults(options, DEFAULT_OPTS);
    }
 
diff --git a/src/utils/common-types.ts b/src/utils/common-types.ts
index ed95855..4f9c984 100644
--- a/src/utils/common-types.ts
+++ b/src/utils/common-types.ts
@@ -6,8 +6,6 @@ export interface StringArrayOfStringsMap { [s: string]: string[] }
 export interface KeyValueStringObject { [k: string]: (string | string[] | KeyValueStringObject) }
 // Removes `readonly` modifier and make all keys writable again
 export type Writable<T> = { -readonly [P in keyof T]-?: T[P] };
-// Makes all keys on an object optional
-export type Optional<T> = { [P in keyof T]? : T[P] };
 
 export function isStringMap(o: any): o is StringMap {
    if (!_.isObject(o) || _.isArray(o)) { // because arrays are objects

@jthomerson jthomerson merged commit 7b947a3 into silvermine:master Mar 25, 2019
@yokuze yokuze mentioned this pull request Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants