LRUCache/LRUMap family -- inspect and toString improvements #187
+538
−99
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
mrflip
force-pushed
the
LRUInspection
branch
3 times, most recently
from
41d5464 to
839c3cf
Compare
* LRUCache and family .inspect limits its output -- showing the youngest items, and ellipsis, and the oldest item. Options allow dumping the raw object or controlling the size of output (and the number of items a console.log will mindlessly iterate over). * LRUCache and family all have inspect wired up to the magic 'nodejs.util.inspect.custom' symbol property that drives console.log output * LRUCache and family all have a summaryString method returning eg 'LRUCache[8/200]' for a cache with size 8 and capacity 200, wired to the magic Symbol.toStringTag property that drives string interpolation (partially addresses Yomguithereal#129).
| @@ -302,6 +303,7 @@ function makeTests(Cache, name) { | |||
| assert.equal(dead, missingMarker); | |||
|
|
|||
| cache.set('one', 'uno'); | |||
|
|
|||
There was a problem hiding this comment.
Given the fact that the maintainer is present but busy, if you want your changes to be merged, I would recommend to split them into smaller code changes like:
- One PR for #.isEqual addition on Set and related test code cleanups
- One PR for isString() improvements
- One PR for inspect() improvements
- One PR for eslint enhancements
That would permit the maintainer to review the code changes broken into more logical pieces
| * If possible, attaching the | ||
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) | ||
| * * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) | ||
| * * the inspect method to Symbol.for('nodejs.util.inspect.custom') (making `console.log(cache)` liveable) |
There was a problem hiding this comment.
Why two level of comments? One is enough I guess.
|
|
||
| /** | ||
| * If possible, attaching the | ||
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) |
There was a problem hiding this comment.
Suggested change
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) | |
| * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) |
| /** | ||
| * If possible, attaching the | ||
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) | ||
| * * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) |
There was a problem hiding this comment.
Suggested change
| * * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) | |
| * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) |
| * If possible, attaching the | ||
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) | ||
| * * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) | ||
| * * the inspect method to Symbol.for('nodejs.util.inspect.custom') (making `console.log(cache)` liveable) |
There was a problem hiding this comment.
Suggested change
| * * the inspect method to Symbol.for('nodejs.util.inspect.custom') (making `console.log(cache)` liveable) | |
| * the inspect method to Symbol.for('nodejs.util.inspect.custom') (making `console.log(cache)` liveable) |
| * If possible, attaching the | ||
| * * #.entries method to Symbol.iterator (allowing `for (const foo of cache) { ... }`) | ||
| * * the summaryString method to Symbol.toStringTag (allowing `\`${cache}\`` to work) | ||
| * * the inspect method to Symbol.for('nodejs.util.inspect.custom') (making `console.log(cache)` liveable) |
| @@ -5,7 +5,7 @@ | |||
| "scripts": { | |||
| "lint": "eslint --cache ./*.js ./utils ./test", | |||
| "lint:fix": "eslint --cache --fix ./*.js ./utils ./test", | |||
| "prepublishOnly": "npm run lint && npm test && npm run test:types", | |||
| "prepublishOnly": "npm run lint && npm test && npm run test:types && echo 'Success! All checks passed!'", | |||
There was a problem hiding this comment.
I doubt that kind of changes is really expected in such a PR :D
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.
The current inspect will put the entire contents of its key/value set onto the screen, causing voluminous dump and/or hiding the end of the contents, and makes it harder to examine its internals. This makes all the LRU classes inherit the method properly; tastefully limits the output while showing both the head and tail; and has an
all: truetrap door to get a shallow copy of the cache as a POJO.