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

Improve handling of nested dependencies #175

Merged
merged 36 commits into from
Dec 13, 2021
Merged

Improve handling of nested dependencies #175

merged 36 commits into from
Dec 13, 2021

Conversation

ncilfone
Copy link
Contributor

@ncilfone ncilfone commented Dec 8, 2021

What does this PR do?

This PR uses simple graph methods to better handle nested dependencies. Also contains a fairly big refactor of some original spaghetti backend code into a much cleaner interface. Mainly contributed by @gbmarc1 via fork. This should close #152.

Checklist

  • Did you adhere to PEP-8 standards?
  • Did you run black and isort prior to submitting your PR?
  • Does your PR pass all existing unit tests?
  • Did you add associated unit tests for any additional functionality?
  • Did you provide documentation (Numpy Docstring format) whenever possible, even for simple functions or classes?

ncilfone and others added 28 commits October 28, 2021 13:34
…ssuming to fall back on defaults instead of recursing the config space to set the correct parameters.
…ng recursed to set config arg values within nested classes.
…ctor should be thought about to handle all the parent/child relations in a cleaner manner
…. Probably a bunch of vestigial code still to remove -- base tests still only 43/48
@ncilfone ncilfone added the enhancement New feature or request label Dec 8, 2021
@ncilfone ncilfone self-assigned this Dec 8, 2021
@ncilfone
Copy link
Contributor Author

ncilfone commented Dec 8, 2021

@gbmarc1 I think you've done a solid job fixing #152 and cleaning up some of my ugly code.

I'm going to plod through and add docstrings for all the new code, but after that I think we should be able to merge to master as long as all the tests run and coverage looks good.

@gbmarc1
Copy link
Contributor

gbmarc1 commented Dec 9, 2021

Sounds good. Thank you for this library!

@coveralls
Copy link

coveralls commented Dec 13, 2021

Pull Request Test Coverage Report for Build 1574694571

  • 421 of 437 (96.34%) changed or added relevant lines in 12 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+2.3%) to 95.295%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spock/backend/spaces.py 23 24 95.83%
spock/args.py 52 56 92.86%
spock/graph.py 58 62 93.55%
spock/backend/field_handlers.py 133 140 95.0%
Files with Coverage Reduction New Missed Lines %
spock/builder.py 1 94.25%
Totals Coverage Status
Change from base Build 1473244521: 2.3%
Covered Lines: 1600
Relevant Lines: 1679

💛 - Coveralls

@ncilfone ncilfone marked this pull request as ready for review December 13, 2021 20:11
@ncilfone
Copy link
Contributor Author

@gbmarc1 Merging into master. Thank you again for all the work on this!

@ncilfone ncilfone merged commit 609122f into master Dec 13, 2021
@ncilfone ncilfone mentioned this pull request Dec 13, 2021
5 tasks
ncilfone added a commit that referenced this pull request Dec 13, 2021
ncilfone added a commit that referenced this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested config values are not updated with provided value in --config file
3 participants