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

feat: adding configureService method for external config options #66

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

jorge-ibm
Copy link
Contributor

@jorge-ibm jorge-ibm commented Oct 22, 2019

Fixes: https://github.ibm.com/arf/planning-sdk-squad/issues/1118

Changes:

  • moved the external configuration code outside of the constructor into it's own method.
  • for compatibility, the constructor calls the new method until we move to the service factory's complete implementation.

The new method will now be invoked from the generator after the BaseService's constructor is called.

Checklist
  • npm test passes (tip: npm run lint-fix can correct most style issues)
  • tests are included
  • documentation is changed or added

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #66 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #66   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           8      8           
  Branches        1      1           
=====================================
  Hits            8      8

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f963eb...1234f12. Read the comment docs.

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dpopp07 dpopp07 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, just two comments.

  1. I think the new method should be public
  2. The commit messages are how the changelog is put together, so the message for this PR needs to be descriptive. It should be the feature you are adding, not the issue on our board you're solving. This message is how users will know about the new feature so use a clear but short summary for the header and then more description in the commit body if needed

lib/base-service.ts Show resolved Hide resolved
Copy link
Member

@dpopp07 dpopp07 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! 👍

@jorge-ibm jorge-ibm changed the title feat: prepare for service factory feat:adding configure service method for external config options Oct 22, 2019
@jorge-ibm jorge-ibm changed the title feat:adding configure service method for external config options feat: adding configure service method for external config options Oct 22, 2019
@jorge-ibm jorge-ibm changed the title feat: adding configure service method for external config options feat: adding configureService method for external config options Oct 22, 2019
@jorge-ibm jorge-ibm merged commit 7324919 into master Oct 22, 2019
ibm-devx-automation pushed a commit that referenced this pull request Oct 22, 2019
# [1.3.0](v1.2.0...v1.3.0) (2019-10-22)

### Features

* adding configureService method for external config options ([#66](#66)) ([7324919](7324919))
@ibm-devx-automation
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@padamstx padamstx deleted the service-factory-node branch November 10, 2020 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants