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

JS-41: make API objects immutable #60

Merged
merged 12 commits into from Aug 21, 2019
Merged

JS-41: make API objects immutable #60

merged 12 commits into from Aug 21, 2019

Conversation

RangerRick
Copy link
Member

@RangerRick RangerRick commented Aug 14, 2019

This PR updates a number of the API objects to be immutable, and reworks the API for interacting with the client and HTTP options and such a little bit as a result.

It also includes a refactor of the mock HTTP implementation(s) as they had kind of become a mess and I wanted to tweak them as I was testing DAO changes.

Pull Request Check Sheet

  • Have you read and followed our Contribution Guidelines?
  • Have you made an issue in the OpenNMS issue tracker?
    If so, you should:
    1. update the title of this PR to be of the format: ${JIRA-ISSUE-NUMBER}: subject of pull request
    2. update the JIRA link at the bottom of this comment to refer to the real issue number
    3. prefix your commit messages with the issue number, if possible
  • Have you made a comment in that issue which points back to this PR?
  • Have you updated the JIRA link at the bottom of this comment to link to your issue?
  • If this is a new feature, is there documentation?
  • If this is a new feature or substantial change, are there tests that cover it?

Pull Request Process

One or more reviewers should be assigned to each PR.

If you know that a particular person is subject matter expert in the area your PR affects, feel free to assign one or more reviewers when you create this PR, otherwise reviewers will be assigned for you.

Once the reviewer(s) accept the PR and the branch passes continuous integration in Bamboo, the PR is eligible for merge.

At that time, if you have commit access (are an OpenNMS Group employee or a member of the Order of the Green Polo) you are welcome to merge the PR.
Otherwise, a reviewer can merge it for you.

Thanks for taking time to contribute!

External References

@RangerRick RangerRick added this to the 2.0.0 milestone Aug 14, 2019
Copy link
Contributor

@mattixtech mattixtech left a comment

Choose a reason for hiding this comment

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

I only skimmed the PR so far, but I'd like to know your thoughts on switching to builder pattern to achieve immutability instead of using the with factory methods. Will review further once we resolve that decision.

src/Client.ts Outdated Show resolved Hide resolved
src/Client.ts Outdated Show resolved Hide resolved
src/Client.ts Outdated Show resolved Hide resolved
src/dao/SituationFeedbackDAO.ts Outdated Show resolved Hide resolved
@RangerRick
Copy link
Member Author

FYI, the force-pushes were just some other cleanup I ran into while testing, but the other new commits are relevant to your feedback, @mattixtech

src/Client.ts Outdated Show resolved Hide resolved
src/Client.ts Outdated Show resolved Hide resolved
src/Client.ts Show resolved Hide resolved
src/api/NestedRestriction.ts Outdated Show resolved Hide resolved
src/api/OnmsHTTPOptions.ts Show resolved Hide resolved
src/api/OnmsHTTPOptions.ts Outdated Show resolved Hide resolved
src/api/OnmsServer.ts Outdated Show resolved Hide resolved
@RangerRick
Copy link
Member Author

I thiiiink this takes care of all your feedback, @mattixtech. Lemme know how it looks.

@RangerRick RangerRick merged commit 2b656e0 into develop Aug 21, 2019
@RangerRick RangerRick deleted the jira/JS-41 branch August 21, 2019 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants