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

Ensure TypeScript is valid in strict mode #4

Merged
merged 2 commits into from Dec 22, 2017

Conversation

Projects
None yet
3 participants
@pimterry
Contributor

pimterry commented Dec 21, 2017

Without this change, a downstream project will fail to compile with strict: true (i.e. the CLI, in my case), because the code here doesn't check for undefined correctly, and doesn't specify a type for this in a case where it can't be inferred.

We don't technically need to use strict everywhere, but I'd like to be able to enable it where we can.

Change-Type: patch

const spaces = _.times(maxLength - prefix.length, _.constant(' ')).join('')
return `${prefix}${spaces} ${message}\n`
}
private getMaxPrefixLength(): number {
private getMaxPrefixLength(): number | undefined {

This comment has been minimized.

@lurch

lurch Dec 22, 2017

Forgive me for wading in on code I've never even looked at before, but I guess this max-prefix-length could be stored as a class-attribute (i.e. an additional property on this), as it will only need to get updated in the constructor and when addPrefix is called, and doesn't need to be re-calculated every time that formatWithPrefix is called ?

@lurch

lurch Dec 22, 2017

Forgive me for wading in on code I've never even looked at before, but I guess this max-prefix-length could be stored as a class-attribute (i.e. an additional property on this), as it will only need to get updated in the constructor and when addPrefix is called, and doesn't need to be re-calculated every time that formatWithPrefix is called ?

This comment has been minimized.

@pimterry

pimterry Dec 22, 2017

Contributor

It could, yes. It's more complicated and a little more error-prone though (e.g. if we add more methods changing the prefixes available, we need to remember to handle this). I'd definitely do that if this was an issue, but right now I'd be fairly surprised if calling getMaxPrefixLength unnecessarily had a measurable effect on performance in any real situation.

So yes, in principle, but until we see a concrete performance cost here I plan to wholeheartedly ignore this otherwise sensible suggestion 😄.

@pimterry

pimterry Dec 22, 2017

Contributor

It could, yes. It's more complicated and a little more error-prone though (e.g. if we add more methods changing the prefixes available, we need to remember to handle this). I'd definitely do that if this was an issue, but right now I'd be fairly surprised if calling getMaxPrefixLength unnecessarily had a measurable effect on performance in any real situation.

So yes, in principle, but until we see a concrete performance cost here I plan to wholeheartedly ignore this otherwise sensible suggestion 😄.

@resin-io-modules-versionbot resin-io-modules-versionbot bot merged commit e97a642 into master Dec 22, 2017

3 checks passed

AutoMerges PR merging is in progress
Reviewers 1/1 review approvals met
Versionist Found all required commit footer tags

@resin-io-modules-versionbot resin-io-modules-versionbot bot deleted the fix-this-typing branch Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment