Skip to content

3482 Add ReferenceResolver to manage config items#3818

Merged
Nic-Ma merged 14 commits intoProject-MONAI:devfrom
Nic-Ma:3482-reference-resolver
Feb 19, 2022
Merged

3482 Add ReferenceResolver to manage config items#3818
Nic-Ma merged 14 commits intoProject-MONAI:devfrom
Nic-Ma:3482-reference-resolver

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Feb 18, 2022 •
edited
Loading

Task step 3-1 of #3482 .

Description

This PR implemented the ReferenceResolver to manage all the ConfigItem and resolve the references.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma Nic-Ma changed the title [WIP] 3482 Add ReferenceResolver to manage config items 3482 Add ReferenceResolver to manage config items Feb 18, 2022
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/build

@Nic-Ma Nic-Ma marked this pull request as ready for review February 18, 2022 09:14
@Nic-Ma Nic-Ma requested review from ericspod, rijobro and wyli February 18, 2022 09:14
@Nic-Ma Nic-Ma mentioned this pull request Feb 18, 2022
Closed
17 tasks
wyli and others added 2 commits February 18, 2022 14:06
@wyli
update docstring (wip)
b6e8005 
Signed-off-by: Wenqi Li <[email protected]>
@Nic-Ma
[DLMED] update id
7847855 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma Nic-Ma force-pushed the 3482-reference-resolver branch from ad2d41f to 7847855 Compare February 18, 2022 15:48
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

Hi @wyli , your change looks good to me.

Thanks.

@Nic-Ma Nic-Ma enabled auto-merge (squash) February 18, 2022 16:49
@wyli
nonbreaking updates
bbe6428 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the 3482-reference-resolver branch from 782a17f to bbe6428 Compare February 18, 2022 17:19
wyli
wyli approved these changes Feb 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks, I made some more non-breaking changes. find_refs_in_config and update_config_with_refs share many similarities, I suspect we can somehow merge these two with an additional 'update: bool` flag. But I think it's also good in its current shape...

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

thanks, I made some more non-breaking changes. find_refs_in_config and update_config_with_refs share many similarities, I suspect we can somehow merge these two with an additional 'update: bool` flag. But I think it's also good in its current shape...

Thanks for your review and update.
I would suggest to keep current shape of the 2 functions, we may revisit and update the logic during development, let's keep it simple and easy to ready.

Thanks.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 18, 2022

/build

@Nic-Ma Nic-Ma disabled auto-merge February 18, 2022 23:54
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 19, 2022

/build

@Nic-Ma Nic-Ma enabled auto-merge (squash) February 19, 2022 00:05
@Nic-Ma Nic-Ma merged commit 384c718 into Project-MONAI:dev Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

@rijobro rijobro Awaiting requested review from rijobro

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Nic-Ma @wyli