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

Custom item validator per folder #617

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

lbiaggi
Copy link
Contributor

@lbiaggi lbiaggi commented Jan 13, 2024

Hi @jacebrowning, we have been using doorstop to manage our ISO 26262 requirements, it is part of our core workflow.

We have improved certain areas of the software to match our requirements, and we think it would be useful to share with upstream these changes.

  • Load a file as a python module
  • A new section inside .doorstop.yml to enable them (we called them extension, as we intend to add more features if necessary)
    - Where we can define a custom validator for the items from a specific folder
    - If file references from this folder should calculate the SHA-256 for them (During review / validate without -W)
    - Change the buffer size for when doing the SHA
  • Override doorstop settings from a file through CLI parameter.

We made these changes because we didn't want to override most of the default behaviour from doorstop, and yet we wanted to be able to tweak certain validations for specific items.

@neerdoc
Copy link
Collaborator

neerdoc commented Jan 15, 2024

@lbiaggi I've had a quick look and it looks very useful! I will do a deeper review and some testing as soon as possible.

Copy link
Collaborator

@neerdoc neerdoc left a comment

Choose a reason for hiding this comment

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

The changes looks good as far as I can see, and would possibly fix #564.

The only issue I have is that there are no tests included for the added functionality.

Please add relevant test cases for the added functionality.

@lbiaggi
Copy link
Contributor Author

lbiaggi commented Jan 22, 2024

The changes looks good as far as I can see, and would possibly fix #564.

The only issue I have is that there are no tests included for the added functionality.

Please add relevant test cases for the added functionality.

Ok!

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 96.16%. Comparing base (b94019a) to head (70f610d).
Report is 2 commits behind head on develop.

Files Patch % Lines
doorstop/core/document.py 57.14% 5 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #617   +/-   ##
========================================
  Coverage    96.15%   96.16%           
========================================
  Files           41       41           
  Lines         5173     5185   +12     
  Branches      1214     1217    +3     
========================================
+ Hits          4974     4986   +12     
+ Misses         127      126    -1     
- Partials        72       73    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lbiaggi lbiaggi force-pushed the extensions branch 4 times, most recently from b1f479c to f0ca9d2 Compare January 22, 2024 15:59
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Jan 22, 2024

Can you re-review @neerdoc? I've inserted tests, updated the sample reqs project and documentation.

@lbiaggi lbiaggi requested a review from neerdoc January 22, 2024 17:01
@lbiaggi lbiaggi force-pushed the extensions branch 12 times, most recently from 5fef8cb to b5651a6 Compare January 24, 2024 14:24
Copy link
Collaborator

@neerdoc neerdoc left a comment

Choose a reason for hiding this comment

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

I've had another look and as far as I can see this is a nice improvement.

It is a rather big change (lot's of new code, I like it!) so I would like to test it a bit more on my own system before we merge it, but good work @lbiaggi!

@jacebrowning should also have a look at it.

@lbiaggi
Copy link
Contributor Author

lbiaggi commented Jan 25, 2024

@neerdoc no worries, I understand.

How can I propose more drastic changes? (I'm looking to improve and expand references type to be more generic and customizable, e.g., to support external links from another repo.) or at least an interface to create custom references.

@neerdoc
Copy link
Collaborator

neerdoc commented Jan 25, 2024

How can I propose more drastic changes? (I'm looking to improve and expand references type to be more generic and customizable, e.g., to support external links from another repo.) or at least an interface to create custom references.

I would suggest that you first read through all open issues to get a good grasp of what other peoples' opinions are regarding where the software is going. Jump in to whatever discussions there already are and voice you suggestions. If you have new ideas, just open an issue on the subject and we will have a discussion about it!

@neerdoc neerdoc requested a review from jacebrowning January 26, 2024 15:18
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Jan 29, 2024

Should I rebase and fix the conflict?

@neerdoc
Copy link
Collaborator

neerdoc commented Jan 29, 2024

Yes please, that would be very helpful!

@lbiaggi lbiaggi force-pushed the extensions branch 2 times, most recently from 2e45de0 to c8cb011 Compare January 29, 2024 15:34
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Jan 29, 2024

Done, if a conflict appears, and I did not update the PR, please ping me

@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 11, 2024

Friendly ping reminder @jacebrowning

@jacebrowning
Copy link
Member

jacebrowning commented Mar 11, 2024

@lbiaggi sorry, I haven't had a chance to look at this until now.

I think we should stick to one feature per pull request (and corresponding issue for discussion, if applicable). Since there are three distinct features here, I'd like to see this split up into three separate pull requests that can be reviewed and applied independently.

@jacebrowning jacebrowning marked this pull request as draft March 11, 2024 19:15
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 12, 2024

OK, but bear in mind that I rely on the first one to do the other two

Can I signalize they are dependent? (at least from the first)

@jacebrowning
Copy link
Member

Yeah, you can identify that they are dependent or just open the base pull request first.

@lbiaggi lbiaggi force-pushed the extensions branch 3 times, most recently from adf2b2d to f34b6a1 Compare March 14, 2024 00:05
@lbiaggi lbiaggi changed the title Multiple changes: custom item validator per folder, override settings through a CLI option, and hash reference per file reference Custom item validator per folder and SHA calc for referenced files Mar 14, 2024
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 14, 2024

@jacebrowning done

@lbiaggi lbiaggi changed the title Custom item validator per folder and SHA calc for referenced files Custom item validator per folder and SHA calc for referenced files PR 2/3 Mar 14, 2024
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 20, 2024

@jacebrowning ping

@jacebrowning
Copy link
Member

@lbiaggi this pull request still seems to contain multiple features: custom item validation and something about changing the SHA that I don't fully understand. I'd like to see this split into two separate pull requests for further review. Thanks!

@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 21, 2024

@lbiaggi this pull request still seems to contain multiple features: custom item validation and something about changing the SHA that I don't fully understand. I'd like to see this split into two separate pull requests for further review. Thanks!

Sorry, IMO they were the same feature because without the SHA, custom validator doesn't seem to be doing something interesting.

I'm not changing the SHA, I'm just calculating the SHA for each file reference...

I will do it now.

@lbiaggi lbiaggi changed the title Custom item validator per folder and SHA calc for referenced files PR 2/3 Custom item validator per folder Mar 21, 2024
@lbiaggi
Copy link
Contributor Author

lbiaggi commented Mar 21, 2024

@jacebrowning done, sorry for misinterpreting what you meant

@lbiaggi lbiaggi marked this pull request as ready for review March 21, 2024 03:24
@jacebrowning jacebrowning merged commit 70f610d into doorstop-dev:develop Apr 4, 2024
16 of 17 checks passed
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.

4 participants