Skip to content

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jul 22, 2020

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 v1 July 22, 2020 17:08
@aeisenberg aeisenberg force-pushed the aeisenberg/ignore branch 2 times, most recently from 012f4da to ac884fe Compare July 22, 2020 18:00
Copy link
Contributor

@robertbrignull robertbrignull left a comment

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.

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
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

// 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`);
Copy link
Contributor

@robertbrignull robertbrignull Jul 24, 2020

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 aeisenberg force-pushed the aeisenberg/ignore branch 2 times, most recently from 23ad9cf to cbe0b21 Compare July 24, 2020 15:07
@aeisenberg aeisenberg changed the base branch from v1 to main July 24, 2020 15:07
@aeisenberg aeisenberg merged commit 93dd64d into github:main Jul 24, 2020
This was referenced Jul 27, 2020
Sign up for free to 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