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.

Sign up for GitHub

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

Jump to bottom

Add yaml tag full support. #1032

Draft
wants to merge 4 commits into
base: rolling
Choose a base branch
from
Draft

Add yaml tag full support. #1032

wants to merge 4 commits into from

Conversation

llapx
Copy link
Contributor

@llapx llapx commented Feb 2, 2023

Signed-off-by: Lei Liu [email protected]

@fujitatomoya
Copy link
Collaborator

@llapx do we have related issue for this? i think we had discussion on supporting yaml tag before, but cannot find the one.

@llapx llapx force-pushed the topic-yaml_tag_full_support branch 3 times, most recently from a8b520e to 42267c4 Compare February 2, 2023 02:27
@llapx
Copy link
Contributor Author

llapx commented Feb 2, 2023

@fujitatomoya

#1026

@llapx llapx mentioned this pull request Feb 2, 2023
Open
@llapx
Copy link
Contributor Author

llapx commented Feb 2, 2023

Can anybody help to handle this Failed ?

>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:

  'Sequence should be of same type. Value type 'bool' do not belong at line_num 1, at /tmp/ws/src/rcl/rcl_yaml_param_parser/src/parse.c:313'

I have tried without this PR (in local), but the error still exist.

Barry-Xu-2018
Barry-Xu-2018 reviewed Feb 2, 2023
View reviewed changes
rcl_yaml_param_parser/src/parse.c Outdated
(0 == strcmp(tag, YAML_STR_TAG)) ||
(0 == strcmp(tag, YAML_INT_TAG)) ||
(0 == strcmp(tag, YAML_FLOAT_TAG)) ||
(0 == strcmp(tag, YAML_TIMESTAMP_TAG)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, TIMESTAMP tag is supported ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's a mistake, I copyed the tags in batch, I have removed it.

rcl_yaml_param_parser/src/parse.c Outdated
*((bool *)*ret_val) = true;
}
ret = RCUTILS_RET_OK;
goto final;
Copy link
Contributor

Choose a reason for hiding this comment

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

goto is unnecessary. Directly do return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

rcl_yaml_param_parser/src/parse.c Outdated
Comment on lines 668 to 669
ret = RCUTILS_RET_OK;
goto final;
Copy link
Contributor

Choose a reason for hiding this comment

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

The same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

rcl_yaml_param_parser/src/parse.c Outdated
Comment on lines 672 to 673
final:
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

return RCUTILS_RET_ERROR;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@Barry-Xu-2018
Copy link
Contributor

I just quick check.
About test, whether incorrect configuration test need to be added ?

@Barry-Xu-2018
Copy link
Contributor

Barry-Xu-2018 commented Feb 2, 2023 •
edited
Loading

I have tried without this PR (in local), but the error still exist.

I run rcl test with latest codes in my local environment. Not find this problem.
Commit b9de174 for rcl repository.

@llapx llapx force-pushed the topic-yaml_tag_full_support branch from 42267c4 to 08b8b2f Compare February 2, 2023 09:20
@llapx
Update code.
ba4046a 
Signed-off-by: Lei Liu <[email protected]>
@llapx
Copy link
Contributor Author

llapx commented Feb 6, 2023

I just quick check. About test, whether incorrect configuration test need to be added ?

This lead to bool res = rcl_parse_yaml_file(path, params_hdl); return False, the error info looks like bellow:

3: [ RUN      ] test_parser.correct_syntax
3: /work/rcl_issues_1026/src/rcl/rcl_yaml_param_parser/test/test_parse_yaml.cpp:53: Failure
3: Value of: res
3:   Actual: false
3: Expected: true
3: Error parsing value hello at line 76, at /work/rcl_issues_1026/src/rcl/rcl_yaml_param_parser/src/parse.c:276
3: [  FAILED  ] test_parser.correct_syntax (0 ms)
3: [----------] 1 test from test_parser (0 ms total)

which may not easy to check.

@llapx
Update code again.
7920f00 
Signed-off-by: Lei Liu <[email protected]>
@llapx llapx force-pushed the topic-yaml_tag_full_support branch from 60224ee to 7920f00 Compare February 7, 2023 01:53
@llapx
Copy link
Contributor Author

llapx commented Feb 8, 2023

@clalancette @fujitatomoya

Can you have a review?

ahcorde
ahcorde requested changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

conflicts

@Barry-Xu-2018
Copy link
Contributor

@ahcorde

I'll take over this. Considering that no one has requested full YAML support at the moment, I plan to put this PR on hold for now. Could you change the status to draft and assign to me?

@ahcorde ahcorde marked this pull request as draft August 26, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Barry-Xu-2018 Barry-Xu-2018 Barry-Xu-2018 left review comments

@ahcorde ahcorde ahcorde requested changes

@audrow audrow Awaiting requested review from audrow

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

@Barry-Xu-2018 Barry-Xu-2018

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@llapx @fujitatomoya @Barry-Xu-2018 @ahcorde