-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
[atomist:generated] [atomist:autofix=typescript_header]
[atomist:generated] [atomist:autofix=typescript_imports]
[atomist:generated] [atomist:autofix=tslint]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement. Feel free to merge, we can address the comments at a later time.
lib/putS3.ts
Outdated
} | ||
|
||
export function escapeSpecialCharacters(filename: string): RegExp { | ||
return new RegExp(filename.replace(/[-\\^$*+?.()|[\]{}]/g, "\\$&") + "$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to clarify this by extracting it into a descriptively-named function. Technically, the escaping of special characters is just the filename.replace(/[-\\^$*+?.()|[\]{}]/g, "\\$&")
part. The rest is turning it into an anchored regular expression. I wonder if we just shouldn't replace this mess with a slice
using the length of the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
test/publishToS3.test.ts
Outdated
import * as assert from "power-assert"; | ||
import { filterKeys } from "../lib/deleteS3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to move the test for this into its own file.
import { PublishToS3Options } from "./options"; | ||
|
||
type FilesAttempted = number; | ||
type SuccessfullyPushedKey = string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You define this two different places. Perhaps define once and import the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the aliases are just for naming, not really doing anything. If they were types with some structure, I would.
|
||
type FilesAttempted = number; | ||
type SuccessfullyPushedKey = string; | ||
type Warning = string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, defined twice.
[atomist:generated] [atomist:autofix=typescript_header]
Fixes #14
and I moved a bunch of functions out of the big function, and then put them in files because it complained about file length