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

fix: unmatching yaml file checking #293

Closed
wants to merge 1 commit into from

Conversation

badai-nguyen
Copy link

@badai-nguyen badai-nguyen commented Apr 26, 2024

Description

  • Regarding hoge.schema.json, CI will check some unreleated .param.yaml files with the filename started by hoge such as hoge_xx.param.yaml, hogeXXX.param.yaml
  • To fix about unneccesary checking.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@badai-nguyen badai-nguyen requested a review from xmfcx April 26, 2024 23:33
@xmfcx
Copy link
Contributor

xmfcx commented Apr 27, 2024

But we actually want to check multiple param files that start with the base_name, that's the point of having a base_param name.

Why do you need this change?

@xmfcx
Copy link
Contributor

xmfcx commented Apr 27, 2024

example usage:
https://github.com/autowarefoundation/autoware.universe/tree/44c6169d1fca13cd1035d459dd127758d5296595/control/joy_controller/config

4 different param.yaml files are managed by a single json schema.

@badai-nguyen
Copy link
Author

@xmfcx Thank you for your comment. I understood that It was intentional setting.
However, i think there are also some cases that more explicity definition of yaml file is needed.
For example, https://github.com/autowarefoundation/autoware.universe/tree/main/perception/lidar_centerpoint/config
If we create centerpoing.schema.json, it will check all other yaml files.
Do you have any suggestion for that?

@xmfcx
Copy link
Contributor

xmfcx commented Apr 27, 2024

@badai-nguyen

If we create centerpoint.schema.json, it will check all other yaml files.

Then, you can avoid this by naming the schema and param files accordingly.

I dont know what those schemas and params do but you could name them according to their purposes:

  • centerpoint_purpose_1.schema.json
    • centerpoint_purpose_1.param.yaml
    • centerpoint_purpose_1_variant_1.param.yaml
    • centerpoint_purpose_1_variant_2.param.yaml
  • centerpoint_purpose_2.schema.json
    • centerpoint_purpose_2.param.yaml
  • centerpoint_purpose_3.schema.json
    • centerpoint_purpose_3.param.yaml

There is no need to change the way it currently works.

Additional benefits

This would also make it easier to understand different kinds of param.yaml files too.

Right now lidar_centerpoint/config folder looks like a mess and it is hard to understand what they all stand for.

@badai-nguyen
Copy link
Author

@xmfcx I see. Then I will close this PR and revise the schemla.

@xmfcx xmfcx deleted the fix/unmatching_file_checking_fix branch June 14, 2024 09:11
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.

2 participants