Sitemap sort order priorities updated #5724
Conversation
| The ``LATEST`` version shall always beat other versions in comparison. | ||
| ``STABLE`` should be listed second. If we cannot figure out the version | ||
| The ``STABLE`` version shall always beat other versions in comparison. | ||
| ``LATEST`` should be listed second. If we cannot figure out the version |
There was a problem hiding this comment.
I don't think we want to make this change here (in this function) since it will affect more places than just the sitemap_xml view.
|
@humitos I have pushed some changes that will easily change the ordering but the problem here is ordering would be like this: is it okay? or should stable be the first one in the list? |
|
@saadmk11 I'm not really sure if that matters. Does the sitemap specification say anything about what's the correct order here? |
|
@humitos The position of a URL in the Sitemap doesn't really matter. |
|
@saadmk11 OK, so I think it's better to sort it by priority. |
|
@humitos Updated the PR. Please have a look. |
readthedocs/core/views/serve.py
Outdated
| @@ -351,7 +351,7 @@ def priorities_generator(): | |||
| It generates values from 1 to 0.1 by decreasing in 0.1 on each | |||
| iteration. After 0.1 is reached, it will keep returning 0.1. | |||
| """ | |||
| priorities = [1, 0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2] | |||
| priorities = [0.9, 1, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2] | |||
There was a problem hiding this comment.
We can leave this list sorted as it was, to avoid confusions.
There was a problem hiding this comment.
if we change this
latest with priority=0.9, changefreq=daily
stable with priority=1, changefreq=weekly
won't work. then we'll have to take another approach. So, I'll try another approach but this seem to have less complexity :)
There was a problem hiding this comment.
Why not changing the order of changefreqs_generators to return weekly first?
There was a problem hiding this comment.
changing the order of changefreqs_generators won't set the priority of stable to 1 and latest to 0.9.
priorities = [0.9, 1, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2]
this is the easiest way to set the priority correctly and also get the changefreq in order.
i.e:
[
{
'loc': 'http://public.readthedocs.io/en/stable/',
'priority': 1,
'changefreq': 'weekly',
'languages': [
..........
]
},
{
'loc': 'http://public.readthedocs.io/en/latest/',
'priority': 0.9,
'changefreq': 'daily',
'languages': [
...........
]
}
]
otherwise we have to look for another solution for this.
There was a problem hiding this comment.
or we have to define a function like this inside the sitemap view which will sort the versions keeping stable as the first position
https://github.com/rtfd/readthedocs.org/blob/900c57532a3199ffdb8975b732eade1be87a6e5c/readthedocs/projects/version_handling.py#L42-L65
even though we are calling it (comparable_version) from here, we have to remove this with the function we create
https://github.com/rtfd/readthedocs.org/blob/900c57532a3199ffdb8975b732eade1be87a6e5c/readthedocs/core/views/serve.py#L385
let me know if there is a better approach then this one or the previous one?
There was a problem hiding this comment.
I was saying this because returning [0.9, 1...] contains some logic hidden in the woods. What we want in the end is assign 1 to stable an 0.9 to latest.
To keep things simple, why not just swapping these two versions from the result returned by sort_version_aware. Like,
sorted_versions = sort_versions_aware(...)
# TODO: add comment explaining why we do this here
sorted_versions[0], sorted_version[1] = sorted_version[1], sorted_version[0]
It's not good either, but at least it does not hide logic and show the problem explicitly.
I'm not sold in any of the "solutions" that I'm proposing. Follow the one that you feel it's better, more legible and maintainable. Whatever you choose, please add a comment in the correct place explaining what's happening and how is the sorting.
There was a problem hiding this comment.
Actually I thought about swapping but it felt like a hacky way and wasn't able find anything simpler to do thats why I was asking you if you know of a better way.
I think we Shouldn’t go for a too complex way to do this small thing. Swapping is simple enough. I think I'll go with your idea with a good comment :)
readthedocs/core/views/serve.py
Outdated
| @@ -379,6 +379,14 @@ def changefreqs_generator(): | |||
| changefreqs = ['daily', 'weekly'] | |||
| yield from itertools.chain(changefreqs, itertools.repeat('monthly')) | |||
|
|
|||
| def sort_by_priority(version_list): | |||
There was a problem hiding this comment.
nit: this argument could be called just versions.
|
@humitos Updated the PR :) |
|
@humitos I have added a check so that we don't get |
|
@humitos it's starting to get conflicts can it be merged? |
closes #5447