Abstract Last-Install.flag file into a reusable class #546
Conversation
nickpape
commented
Mar 1, 2018
nickpape
commented
- Introduce a last-install.flag API
- Also, fix a bug with installing the package manager
| /** | ||
| * A helper class for managing last-install flags, which are persistent and | ||
| * indicate that something installed in the folder was sucessfully completed. | ||
| * It also compares state, so that if something like the Node JS version has changed, |
There was a problem hiding this comment.
Node JS [](start = 57, length = 7)
Node.js #Resolved
| * it can invalidate the last install. | ||
| * @internal | ||
| */ | ||
| export class LastInstallFlag { |
There was a problem hiding this comment.
LastInstallFlag [](start = 13, length = 15)
Add a unit test #Resolved
| * @param folderPath - the folder that this flag is managing | ||
| * @param _state - optional, the state that should be managed or compared | ||
| */ | ||
| constructor(folderPath: string, private _state: Object = {}) { |
There was a problem hiding this comment.
private [](start = 34, length = 7)
Never use this language feature. It hides the data fields, which are arguably the most important aspect of a class. (The thesis of object-oriented programming is to focus on data instead of operations; if the nouns are clearly defined then the interpretation of the verbs will be obvious.) #Resolved
| */ | ||
| public isValid(): boolean { | ||
| try { | ||
| return _.isEqual(fsx.readJsonSync(this._flagPath), this._state); |
There was a problem hiding this comment.
readJsonSync [](start = 27, length = 12)
Test whether the file exists, to avoid an "expected exception" #Resolved
| /** | ||
| * Writes the flag file to disk with the current state | ||
| */ | ||
| public set(): void { |
There was a problem hiding this comment.
set [](start = 9, length = 3)
"set" is a keyword. Also it's not super clear.
Maybe "create"? #Resolved
| /** | ||
| * Returns the full path to the flag file | ||
| */ | ||
| public get flagPath(): string { |
There was a problem hiding this comment.
flagPath [](start = 13, length = 8)
lastInstallFlag.flagPath seems redundant, maybe just "path"? #Resolved
| * it can invalidate the last install. | ||
| * @internal | ||
| */ | ||
| export class LastInstallFlag { |
There was a problem hiding this comment.
LastInstallFlag [](start = 13, length = 15)
I feel like the class name should have the word "file' in it, since that's a pretty important aspect of this API. (Then maybe the verbs could be something like create/delete/test instead of set/clear?) #WontFix
There was a problem hiding this comment.
We can fix this later if we decide to generalize this API
In reply to: 171727085 [](ancestors = 171727085)
| */ | ||
| public isValid(): boolean { | ||
| try { | ||
| return _.isEqual(fsx.readJsonSync(this._flagPath), this._state); |
There was a problem hiding this comment.
fsx.readJsonSync(this._flagPath) [](start = 23, length = 32)
for debugging purposes, it might be useful to store the readJsonSync() result in a local variable #Resolved
