Skip to content

Use templated strings for better readability - #121

Merged
aeisenberg merged 1 commit into
github:mainfrom
aeisenberg:aeisenberg/ignore
Jul 24, 2020
Merged

Use templated strings for better readability#121
aeisenberg merged 1 commit into
github:mainfrom
aeisenberg:aeisenberg/ignore

Conversation

@aeisenberg

@aeisenbergaeisenberg commented Jul 22, 2020

Copy link
Copy Markdown
Contributor

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@aeisenberg
aeisenberg changed the base branch from main to v1July 22, 2020 17:08
@aeisenberg
aeisenbergforce-pushed the aeisenberg/ignore branch 2 times, most recently from 012f4da to ac884feCompareJuly 22, 2020 18:00

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple of changes to the wording of messages which I don't feel are improvements. But overall thanks this looks good.

I'l leave it to you to make the suggested changes or not, and merge when you're happy.

Comment threadsrc/fingerprints.ts Outdated
location.index >= artifacts.length ||
typeof artifacts[location.index].location !== 'object') {
core.debug('Ignoring location as index "' + location.index + '" is invalid');
core.debug(`Ignoring location URI "${location.index}" is invalid`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The text changed here too which looks like a mistake

Comment threadsrc/fingerprints.ts Outdated
// Get the URI and decode
if (typeof location.uri !== 'string') {
core.debug('Ignoring location as uri "' + location.uri + '" is invalid');
core.debug(`Ignoring location URI "${location.uri}" is invalid`);

@robertbrignullrobertbrignullJul 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The removal of the word "as" makes this harder to read, though I admit it is an error message so perfect grammar is not of the greatest importance. Still I think the removal as "as" is negative change.

@aeisenberg
aeisenbergforce-pushed the aeisenberg/ignore branch 2 times, most recently from 23ad9cf to cbe0b21CompareJuly 24, 2020 15:07
@aeisenberg
aeisenberg changed the base branch from v1 to mainJuly 24, 2020 15:07
@aeisenberg
aeisenberg merged commit 93dd64d into github:mainJul 24, 2020
This was referenced Jul 27, 2020
Sign up for freeto 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.

2 participants

@aeisenberg@robertbrignull