-
Notifications
You must be signed in to change notification settings - Fork 42
implement new namespace patterns based on ABNF #882
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
Conversation
.. as it comes from working file `s3.abnf` from #858 (comment) and one intermediate and two trailing spaces are unnecessary
use valid examples where it is fairly obvious what the meaning of the test is. Remove tests where it is not or the test does not seem useful anymore. (The pattern schema is not yet adapted and thus checks via the schema will fail.)
in code that the new ssvc pattern touched
tschmidtb51
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.
First half of my review - main point:
Please use x_example.test#test instead.
docs/reference/code/namespaces.md
Outdated
| `x_org.example#bar`, and there are no guarantees of global uniqueness for the | ||
| decision points in the `x_org.example#bar` namespace. |
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.
| `x_org.example#bar`, and there are no guarantees of global uniqueness for the | |
| decision points in the `x_org.example#bar` namespace. | |
| `x_example.test#test`, and there are no guarantees of global uniqueness for the | |
| decision points in the `x_example.test#test` namespace. |
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.
@ahouseholder Or we rephrase that to x_example.test#documentation
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.
I worked on that part - please have a look in the namespaces.md during your review.
.. by fixing a typo in a test example. .. by adding whitespace after [ and before ] to be consistent.
following suggestion in #882 (comment)
following suggestion #882 (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.
holding my review until @tschmidtb51's concerns have been resolved.
|
tschmidtb51
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.
@bernhardreiter Please address my comments.
@ahouseholder There are some questions in this review for you: Could you please answer them?
- addresses parts of #858 - update decision to match current ABNF - minor format updates and clarifications
|
@ahouseholder: How do we call a collection of decision points in one namespace? It is not necessarily a decision point group... Currently, I use "model" but I guess there should be a better wording for that. |
make sure pattern is ended and correct example
…nto namespacepattern
remove code that added an anchor to fix a test
after previous changes that anchored the ssvc pattern
Co-authored-by: tschmidtb51 <[email protected]>
Co-authored-by: tschmidtb51 <[email protected]>
Co-authored-by: tschmidtb51 <[email protected]>
I'd suggest collection. We use model interchangeably with the thing we now call a |
to `.example.test#test` from `.com.example#foo` and other combinations.
|
@tschmidtb51 I think I've addressed all your comments directed at me. Please let me know if I've missed anything. |
tschmidtb51
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.
Still some minors.
Co-authored-by: tschmidtb51 <[email protected]>
Co-authored-by: tschmidtb51 <[email protected]>
remove old textual description in favor of a link to the ABNF, which is a more precise description. Avoid duplication this way.
6a0bc7e improved the documentation in two places. It mainly links to the new ABNF file and avoids some duplication this way. |
- addresses review comments of #882 - update a few examples - corrected "model" to "collection"
tschmidtb51
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.
I think we addressed all comments so far - @ahouseholder Please review.
|
Note that docs/reference/code/namespaces.md would still need more love before it is a good documentation. It partly explains the ABNF, mixed with meaning. This would need to be cross checked, then stripped of duplicates, corrected for errors found in the process and then checked in the mkdocs layout. |
|
The link checker fails can be fixed but the |
Note that this test failure was there before this PR, so the PR does not worsen the situation. |
src/ssvc/utils/ssvc_namespace_pattern.abnf.The location was chosen to be as close to the python file where it is integrated.
patterns.pywith ones generated from the ABNFGeneration was done with
on Debian GNU/Linux Bookworm.
based on #881, so that tests run in principle.
One failing test was there before and is untouched.
blackwas hinted upon in the repository, so the changed code was run by it.A review should take a deeper look at the changed testcases in test_namespaces_pattern.py
and note that docs/reference/code/namespaces.md is know to need to be overhauled with the results of the discussions that lead to #858.
(The documentation has not been adapted further as the reasoning behind the #858 are not fully documented in that issue.)
Assuming shared copyright on the new file,
ssvc_namespace_pattern.abnf. As I am working on behalf of @tschmidtb51, the (German) BSI owns the copyright of copyrightable changes.Additional suggestions for further improvements (out of scope for this new code):
This means extending the requirements.
resolve #858