fix: redundant word in banner for Legacy stream documents#7207
fix: redundant word in banner for Legacy stream documents#7207rjsparks merged 3 commits intoietf-tools:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7207 +/- ##
==========================================
- Coverage 88.78% 88.77% -0.02%
==========================================
Files 296 296
Lines 41320 41338 +18
==========================================
+ Hits 36687 36698 +11
- Misses 4633 4640 +7 ☔ View full report in Codecov by Sentry. |
| { | ||
| "fields": { | ||
| "desc": "Legacy stream", | ||
| "desc": "Legacy", |
There was a problem hiding this comment.
This won't actually change production. it will show up here as a side-effect of the change in the database instead. The PR will need a data migration to change the name.
There was a problem hiding this comment.
Got it. It will take me a few days to re-understand the migration stuff. If you want to add a commit or hints to me, feel free. (Not asking, just not wanting to be a blocker if I don't have to be)
There was a problem hiding this comment.
No worries, this can wait awhile
NGPixel
changed the title
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Just a minor nit - I'm ok with committing as is or making the change.
| else: | ||
| stream_desc = "(None)" | ||
| stream = "(None)" | ||
| stream = ("draft-stream-" + doc.stream.slug) if doc.stream != None else "(None)" |
There was a problem hiding this comment.
nit: "is not None" instead of "!= None"
There was a problem hiding this comment.
There are seven "!= None" and four "is not None" in the file. Let me know if you want to standardize on one.
There was a problem hiding this comment.
Should definitely be is not None (citation: https://peps.python.org/pep-0008/#programming-recommendations)
We're not obsessively zealous about it, but I try to rephrase things as I encounter them and it'd be great if you don't mind cleaning these up!
fix: Change "Legacy stream" to "Legacy" chore: Add "stream" to stream.desc as needed Fixes: ietf-tools#6902
The stream_desc key isn't used in template/doc/docuemnt_draft.html to don't pass it in nor compute it Fixes: ietf-tools#6902
richsalz
dismissed stale reviews from jennifer-richards and rjsparks
via
1f807bb
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Having a little trouble following the history because of the force pushing, but it looks like at some point there had been a migration to make the change on production that Robert had suggested and it was taken out?
I think it needs that unless we're planning to make the change through the admin, which would not be unreasonable since it's a single value.
Aside from that, lgtm.
|
I built and pushed a migration - I suspect Rich missed that and didn't merge back into his branch. |
|
@rjsparks if you push the commit, then I'll probably leave this untouched as @jennifer-richards basically approved and I'll just keep my hands off :) |
|
for other PRs - if someone else commits to richsalz/datatracker |
|
migration cherry-picked back into the PR |
sorry for that. Old habit I picked up with OpenSSL, the first place I learned git. |
3 participants
This does all the changes suggested by @rjsparks in #6902 and a little bit more cleanup