Skip to content
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

fetcher: rename service.name to service.resource #216

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

pablopalacios
Copy link
Contributor

Closes #51

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@pablopalacios pablopalacios force-pushed the rename-name-to-resource branch 2 times, most recently from 97d7f79 to 8e7d128 Compare November 2, 2020 17:24
Copy link
Contributor

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a couple of comments.

README.md Outdated Show resolved Hide resolved
libs/fetcher.js Outdated
resourceName = fetcher.resource;
} else if (typeof fetcher.name !== 'undefined') {
if ('production' !== process.env.NODE_ENV) {
console.warn('"fetcher.name" is deprecated. Please rename it to "fetcher.resource".');
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be spammy in the console since we have many fetchers that use name. if we could only deprecate it once when the server starts, that would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved this logic to Fetcher.middleware. I also added an array to control all the deprecated services definitions. In this way, it will be easier to spot services non compliant with the newer API.

@coveralls
Copy link

coveralls commented Nov 2, 2020

Coverage Status

Coverage decreased (-0.3%) to 97.053% when pulling 4e97035 on pablopalacios:rename-name-to-resource into 323f31b on yahoo:master.

Copy link
Contributor

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@redonkulus redonkulus merged commit 6f1a309 into yahoo:master Nov 3, 2020
@pablopalacios pablopalacios deleted the rename-name-to-resource branch November 3, 2020 16:29
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.

Rename services' 'name' property to 'resource'
3 participants