-
Notifications
You must be signed in to change notification settings - Fork 19
Improve check_id_format #192
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
Improve check_id_format #192
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
| if id_parts_len != expected_parts: | ||
| if expected_parts == 2: | ||
| msg = "expected to consist of this format: `<Req Type>__<Abbreviations>`." | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| else: | |
| elif expected_parts == 3: |
and some generic else afterwards. Either generic or some error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Please resolve.
|
|
||
| if id_parts_len != expected_parts: | ||
| if expected_parts == 2: | ||
| msg = "expected to consist of this format: `<Req Type>__<Abbreviations>`." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it also would make sense to say it in more plain english as well?
Like:
expected to consist of this format: <Req Type>__<Abbreviations>. Only one '__' is allowed in this need type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Please resolve.
|
The list as described in the Metamodel is correct. (Looked through together with Process). Will now this coming Thursday (7.08) fix errors in Process & Score. |
|
/consumer-test |
1 similar comment
|
/consumer-test |
|
@Aymen-Soussi-01 could you maybe put an exception into the 'feature_id' check for |
|
Why don't they use Should we exclude "example_feature"? Or even "example_*"? Only in process repo? Only for this one check? |
Probably example_*? though not sure. I guess this needs to be discussed. |
|
@Aymen-Soussi-01 Can you add this to this PR too? this is also in 'id_contains.py' I think |
| max_lenght = 45 | ||
| parts = need["id"].split("__") | ||
| if parts[1] == "example_feature": | ||
| max_lenght += 15 # '_example_feature_' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| max_lenght += 15 # '_example_feature_' | |
| max_lenght += 15 # 'example_feature' |
Forgot to change it here too, I only did it in the desc. Sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Please resolve.
| if len(need["id"]) > max_lenght: | ||
| msg = ( | ||
| f"exceeds the maximum allowed length of 45 characters " | ||
| f"(current length: {len(need['id'])})." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and we probably need to also remove the 15 lenghts from the length here if the part[1] is 'example_feature` otherwise in those cases this error will be off by 15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God, so much missed on my part. Sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Please resolve.
…emoving the hardcoded need in the check, then adapt tests
390c2ef to
b84e9ff
Compare
|
The created documentation from the pull request is available at: docu-html |
|
/consumer-test |
|
|
||
| # Get the part of the string after the first two underscores: the path | ||
| feature = parts[1] | ||
| if feature == "example_feature": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a "note" to the requirements? separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#201 We have to fix many links anyway that are currently scattered everywhere
AlexanderLanin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except override
|
Tests & Building of Docs passed locally with git override to Process PR. Should be all fine now I would say. |
|
@AlexanderLanin you should be able to approve it. |
9a8eced
into
eclipse-score:main
Improve check_id_format by adding parts option in the metamodel and removing the hardcoded need in the check, then adapt tests
close: #145