Skip to content

3482 Add support for relative IDs in the config content#3974

Merged
wyli merged 13 commits intoProject-MONAI:devfrom
Nic-Ma:3482-add-relative-ids
Mar 23, 2022
Merged

3482 Add support for relative IDs in the config content#3974
wyli merged 13 commits intoProject-MONAI:devfrom
Nic-Ma:3482-add-relative-ids

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Mar 22, 2022

Task step 12 of #3482 .

Description

According to the task plan, this PR added support for relative reference ID, like: "test": "@###data#1".

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 3482 Add support for relative ids in the config content 3482 Add support for relative IDs in the config content Mar 22, 2022
@Nic-Ma Nic-Ma mentioned this pull request Mar 22, 2022
Closed
17 tasks
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/build

1 similar comment
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/build

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 22, 2022

I think this makes it difficult to distinguish the empty id "" and the parent reference?

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

I think this makes it difficult to distinguish the empty id "" and the parent reference?

Hi @wyli ,

The regex in this PR is re.compile(rf"{ref}{sep}+"), which only match the relative IDs at beginning of the string.

Thanks.

wyli
wyli reviewed Mar 22, 2022
View reviewed changes
wyli
wyli reviewed Mar 22, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 22, 2022

/build

@Nic-Ma Nic-Ma requested review from ericspod, rijobro and wyli March 22, 2022 15:19
ericspod
ericspod reviewed Mar 22, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 23, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 23, 2022

/build

@Nic-Ma Nic-Ma requested a review from ericspod March 23, 2022 00:32
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 23, 2022

Hi @wyli @ericspod ,

Thanks for your review and suggestions.
I revisited the feature, actually, the relative id conversion is kind of "macro replacement" and we need to support it for both the macro token %XXX case and reference @XXX case. So I changed the logic to ConfigParser and no need to call it twice now.
Could you please help review it again?

Thanks in advance.

@wyli wyli merged commit fbd09a1 into Project-MONAI:dev Mar 23, 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

@rijobro rijobro Awaiting requested review from rijobro

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Nic-Ma @wyli @ericspod