Skip to content

support for nested references #146

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

Closed
wants to merge 1 commit into from
Closed

Conversation

dreisel
Copy link

@dreisel dreisel commented Jan 19, 2020

This PR allows for nested references:
for example:

{
    "Nats": {
      "$ref": "common://nats",
      "RetryIntervalMs": {
        "$ref": "common://pool:PoolSize"
      }
    }
  }

@coveralls
Copy link

coveralls commented Jan 19, 2020

Pull Request Test Coverage Report for Build 374

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.2%) to 93.851%

Totals Coverage Status
Change from base Build 371: 1.2%
Covered Lines: 673
Relevant Lines: 706

💛 - Coveralls

@dreisel
Copy link
Author

dreisel commented Feb 26, 2020

@JamesMessinger What do you think about this change?

@JamesMessinger
Copy link
Member

Hi @dreisel. Thank you for your PR. Apologies for the long-delayed response 🙏

I like this new feature, and I don't see any problems with the code off-hand, but it definitely needs some tests. I recommend creating a test/specs/nested/nested.spec.js file and basically duplicating the contents & structure of the test/specs/external folder, but with some nested $refs to test your new functionality. Be sure to test nested $refs that point to external files as well as nested $refs that point to other locations in the same file.

@alex-y-su
Copy link

Hello, any news on this pull requests? If tests are only concerns I will create atoner pull requests with test. Thanks.

@philsturgeon
Copy link
Member

@alexeysuvorov hello! 👋 No need for another pull request, you can push tests into this same branch and it'll update this PR.

Without those tests it's a little tricky for me to see what exactly is going on, which means it's hard to say if I have concerns or not. Can we make sure the change is working as you intended first then go from there?

@philsturgeon
Copy link
Member

Closing for inactivity. If you can bung some tests into this PR that would help us get this merged.

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.

5 participants