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

Remove master json when diaggregate json tests #1627

Merged
merged 4 commits into from
Feb 24, 2025

Conversation

weilixu
Copy link
Collaborator

@weilixu weilixu commented Jan 27, 2025

No description provided.

@@ -521,6 +530,8 @@ def write_ruletest_json(section, rule, ruleset_doc):

# Write out final rule dictionary
write_ruletest_json(prev_section, prev_rule, ruleset_doc)
# Remove the master json file:
os.remove(master_json_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking now that having this master json exist in the repo will be helpful for the rule renumbering process. Because the spreadsheets are not part of the repo, I want to have the renumbering script be able to renumber all test jsons independent of the spreadsheets. I am imagining that the spreadsheets and master json will only use the unique id so that they have no work necessary when renumbering. The disaggregate function will handle the sectionNruleM mapping and renumber the disaggregated rule tests with the appropriate values for N and M.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@weilixu @jugonzal07 does this sound like a good workflow to you? The only drawback that I see is that it makes it difficult for a developer to look at the spreadsheet and know which rules the tests correspond to.

I think there is also an automated solution to this drawback where the rule id map can be written into a dedicated spot of each of the spreadsheets so that each spreadsheet can perform the same mapping and display the Section N Rule M (for developer convenience only - the row would be ignored by the Excel to master JSON script)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to keep the master jsons in the repo but have them excluded from the build?

Copy link
Collaborator Author

@weilixu weilixu Feb 6, 2025

Choose a reason for hiding this comment

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

The workflow works good. I can make the code only run in build process (when releasing to PYPI). I think that was one of my intentions.

I will remove this line in the json_generation_utilities file.

@weilixu weilixu merged commit 58f9665 into develop Feb 24, 2025
2 checks passed
@weilixu weilixu deleted the remove_master_json_when_diaggregate_json_tests branch February 24, 2025 19:34
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