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/stringify codes #459

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Fix/stringify codes #459

merged 3 commits into from
Sep 7, 2023

Conversation

tab1tha
Copy link
Collaborator

@tab1tha tab1tha commented Sep 7, 2023

The previous change made on this branch, to change the rule codes to strings at the point where they are read into the registry, caused the in-rule-file tests to fail.
This was because, the type conversion happened in the validation func so that by the time the test_validation function was being run, the rule code had changed already.
For example, for a rule defined with code=100 (int) the @rule_definition decorator will read it in, convert, then store it as a string. such that by the time we get to the test_validate function,
assert rule_code == 100 will fail. instead we'd have to do assert rule_code= "100"

Considerations:

  • The goal is to have rule codes all as strings by the time they reach the frontend. It is fairly straightforward to convert the rule_code column to str just before sending it over. However, isn't intuitive to a new developer who enters the codebase.
  • If we want rule codes as strings then they should only be entered as strings. Not strings sometimes and ints other times. Consistency is a good thing to lean toward.

The changes in this pull request implement the second consideration and updates the type expectation in the definition of rule_definition such that it expects only str rule codes and will flag if otherwise.

@codecov-commenter
Copy link

Codecov Report

Merging #459 (5e9285c) into main (f9495f4) will decrease coverage by 0.09%.
Report is 1 commits behind head on main.
The diff coverage is 94.56%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
- Coverage   93.13%   93.05%   -0.09%     
==========================================
  Files         123      123              
  Lines        5083     5096      +13     
==========================================
+ Hits         4734     4742       +8     
- Misses        349      354       +5     
Files Changed Coverage Δ
cin_validator/cin_validator.py 12.85% <0.00%> (-0.10%) ⬇️
cin_validator/rule_engine/__registry.py 100.00% <ø> (ø)
cin_validator/rules/cin2022_23/rule_2883.py 100.00% <ø> (ø)
cin_validator/__main__.py 53.48% <69.23%> (+2.13%) ⬆️
cin_validator/rules/cin2022_23/rule_100.py 100.00% <100.00%> (ø)
cin_validator/rules/cin2022_23/rule_1103.py 100.00% <100.00%> (ø)
cin_validator/rules/cin2022_23/rule_1104.py 100.00% <100.00%> (ø)
cin_validator/rules/cin2022_23/rule_1105.py 100.00% <100.00%> (ø)
cin_validator/rules/cin2022_23/rule_1510.py 100.00% <100.00%> (ø)
cin_validator/rules/cin2022_23/rule_1520.py 100.00% <100.00%> (ø)
... and 72 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tab1tha
Copy link
Collaborator Author

tab1tha commented Sep 7, 2023

Again, how were these failing tests not caught when the earlier pull request was made?
Somehow they managed to pass Github automated testing.

@tab1tha tab1tha merged commit 6783594 into main Sep 7, 2023
6 checks passed
@tab1tha tab1tha deleted the fix/stringify-codes branch September 7, 2023 16:56
@SLornieCYC
Copy link
Contributor

Again, how were these failing tests not caught when the earlier pull request was made? Somehow they managed to pass Github automated testing.

Are the github actions configured to checkout the relevant branch when they run? I think actions/checkout uses the repository main branch unless told otherwise.

@tab1tha
Copy link
Collaborator Author

tab1tha commented Sep 21, 2023

Thank you for digging into this @SLornieCYC
The documentation you linked to is helpful. I've gone through, and it seems that if we implement it to point to the branch we're on, we'll have to edit the yaml file each time we create a new branch.
Initially, it seemed that writing my-branch as is done in the tutorial will guess what branch you are on but when I searched the repo, I couldn't find any reference to a my-branch variable apart from the one stated in the documentation. So it is a placeholder that has to be manually edited each time.

However, if we leave it to run on on the main branch, as it does now, then it checks if the incoming changes might cause the main branch to fail. This works sometimes (since some pull requests have failed tests while being on non-main branches) but seems to not work all the time.

@SLornieCYC
Copy link
Contributor

Can't you use the github environment variable there in place of my-branch?

E.g. something like: (Not 100% sure on the right syntax)

uses: actions/checkout@v4
with:
ref: ${{ github.head_ref || github.ref_name }}

https://stackoverflow.com/questions/60300169/how-to-get-branch-name-on-github-action
https://stackoverflow.com/a/71158878

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.

3 participants