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.

Sign up for GitHub

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

Jump to bottom

fix: Include missing related drafts in IPR searches #7836

Merged
merged 6 commits into from
Sep 10, 2024
Merged

fix: Include missing related drafts in IPR searches #7836

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1147c4f
fix: Include missing related drafts in IPR searches
microamp Aug 16, 2024
f643b1c
Merge branch 'main' into 7824-ipr-search
rjsparks Aug 29, 2024
62a52ee
refactor: extract drafts, sort docs
rjsparks Aug 29, 2024
195f8cb
chore: indent loop and conditionals to improve readability
microamp Sep 10, 2024
6e4c803
test: handle whitespaces added to IPR search result page
microamp Sep 10, 2024
a4fc313
Merge branch 'main' into 7824-ipr-search
microamp Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions ietf/ipr/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,18 +689,41 @@ def search(request):
if len(start) == 1:
first = start[0]
doc = first
docs_related = related_docs(first, reverse_relationship=())
drafts_related = [
d2
for d1 in docs_related
for d2 in related_docs(d1, relationship=())
if d2 is not None
]
docs = list(set(docs_related + drafts_related))
iprs = iprs_from_docs(docs,states=states)
docs = set([first])
docs.update(
related_docs(
first, relationship=("replaces", "obs"), reverse_relationship=()
)
)
docs.update(
set(
[
draft
for drafts in [
related_docs(
d, relationship=(), reverse_relationship=("became_rfc",)
)
for d in docs
]
for draft in drafts
]
)
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No strong preference here, but I personally find it easier to read when fors are kept flat like below.

docs.update(
    set(
        draft
        for d in docs
        for draft in related_docs(d, ...)
    )
)

Let it be a team decision though.

Copy link
Member

Choose a reason for hiding this comment

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

It should be whatever Black style says, which I think is what you suggest but not certain

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now that I'm at a keyboard I see that this is more about the brackets than about the styling per se so appealing to Black isn't quite enough...

Copy link
Member

Choose a reason for hiding this comment

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

well, what's there is what black produced.

Copy link
Collaborator Author

@microamp microamp Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, now that I'm at a keyboard I see that this is more about the brackets than about the styling per se

Correct. I wasn't referring to formatting style - sorry for the confusion. It was more about whether the new code is actually more readable despite having one more for in the expression. My opinion is that it isn't.

Anyway, I'm okay with either as long as you're happy with the logic itself.

docs.discard(None)
docs = sorted(
docs,
key=lambda d: (
d.rfc_number if d.rfc_number is not None else 0,
d.became_rfc().rfc_number if d.became_rfc() else 0,
),
reverse=True,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice. Food for thought (not requesting a change here) - we can even do something like

key=lambda d: (
    d == first,  # to ensure that the doc selected will always be first (untested)
    # the rest...
)

The expression will return a bool value which is just an int in Python. i.e.

assert isinstance(True, int)

Copy link
Member

Choose a reason for hiding this comment

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

I think 0 if d == first else 1 would be safer since it doesn't rely on knowing about the Python internals (and because I think it'd have to be d != first because

>>> False < True
True

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(and because I think it'd have to be d != first because

>>> False < True
True

We have reverse=True as an argument to the sorted function above. So, I believe d == first is the correct one.

assert sorted([False, True], reverse=True) == [True, False]

Anyway, I'll leave the existing code as-is, leaving the template to handle that logic for now.

)
iprs = iprs_from_docs(docs, states=states)
template = "ipr/search_doc_result.html"
updated_docs = related_docs(first, ('updates',))
related_iprs = list(set(iprs_from_docs(updated_docs, states=states)) - set(iprs))
updated_docs = related_docs(first, ("updates",))
related_iprs = list(
set(iprs_from_docs(updated_docs, states=states)) - set(iprs)
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nitpick: Formatting changes can be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I ran black on the unit. I could have done it in a separate commit.

# multiple matches, select just one
elif start:
docs = start
Expand Down
Loading