feat: support iab and iesg statements. Import iab statements.#5895
feat: support iab and iesg statements. Import iab statements.#5895rjsparks merged 19 commits intoietf-tools:feat/iabstatementsfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/iabstatements #5895 +/- ##
=====================================================
Coverage ? 88.67%
=====================================================
Files ? 290
Lines ? 40166
Branches ? 0
=====================================================
Hits ? 35619
Misses ? 4547
Partials ? 0 |
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
I have a handful of comments, all minor, inline, and optional.
| # ] | ||
|
|
||
| DOCUMENT_PATH_PATTERN = '/assets/ietf-ftp/{doc.type_id}/' | ||
| DOCUMENT_PATH_PATTERN = '/assets/ietfdata/doc/{doc.type_id}/' |
There was a problem hiding this comment.
These sorts of files are still in /assets/ietf-ftp on my system - do we need to update the dev images to match this?
There was a problem hiding this comment.
There will be other fallout for the dev system, but this needs to happen.
| try: | ||
| _ = markdown.markdown(markdown_content) | ||
| except Exception as e: | ||
| raise forms.ValidationError(f"Markdown processing failed: {e}") |
There was a problem hiding this comment.
Might be better to log the exception instead of (or in addition to) exposing it to the UI, particularly if you're catching Exception instead of something more specific.
There was a problem hiding this comment.
well, what Exception might get thrown in this case that isn't "Markdown processing failed"?
It's exposed in the UI so the submitter can edit whatever they're sending in that markdown is blowing up on.
| @@ -0,0 +1,29 @@ | |||
| $(document) | |||
There was a problem hiding this comment.
Adding jQuery makes @NGPixel cry. I don't object to duplicating a small bit like this, but better to wrap it in $(...) instead of the deprecated $(document).ready(...).
There was a problem hiding this comment.
The existing thing that this is just a copy/paste of would also need to be changed.
A later refactor to remove the duplication would be useful.
3 participants
I think we should merge this into a feature branch first and let the secretariat explore it before merging it to main, but would like some review before we do even that.