5

Github stop displaying and serving authorship information by hi-rustin · Pull Re...

 3 years ago
source link: https://github.com/rust-lang/docs.rs/pull/1322
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

Conversation

Copy link

Contributor

hi-rustin commented 14 days ago

edited

Copy link

Member

jyn514 commented 14 days ago

This is wrong, it also prevents viewing pages by the owner. It should only remove get_releases_by_author.

Copy link

Contributor

Author

hi-rustin commented 14 days ago

@jyn514 Do we need to rename these api and routing parameters?

Copy link

Member

Nemo157 left a comment

I think it would be good to rename where appropriate, but that seems less important to be done straight away. We also need to stop recording the data we're no longer displaying, and then delete the existing data from the database. I think it would make sense to open a local tracking issue here so those steps can be done independently.

templates/crate/details.html

Outdated

Show resolved

Copy link

Member

jyn514 commented 13 days ago

edited

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

Copy link

Contributor

Author

hi-rustin commented 13 days ago

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

hi-rustin

force-pushed the

hi-rustin:rustin-author

branch 2 times, most recently from 9792196 to 7868d15

13 days ago

Copy link

Contributor

Author

hi-rustin commented 13 days ago

edited

@jyn514 Thank you for your review.

Copy link

Member

jyn514 commented 13 days ago

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

Sorry, I meant to say "an author instead of an owner". The existing tests are only for when the owner is found, not when there's an error.

hi-rustin

force-pushed the

hi-rustin:rustin-author

branch 2 times, most recently from 4084b1e to ff55cba

13 days ago

Copy link

Contributor

Author

hi-rustin commented 13 days ago

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

Sorry, I meant to say "an author instead of an owner". The existing tests are only for when the owner is found, not when there's an error.

Add nonexistent_owner_page for it.

Copy link

Contributor

Author

hi-rustin commented 13 days ago

@jyn514 I think it's ready for review.

Copy link

Contributor

Author

hi-rustin commented 13 days ago

@rustbot label: -S-waiting-on-author +S-waiting-on-review

Copy link

Member

jyn514 commented 12 days ago

Add nonexistent_owner_page for it.

Thanks. That shows the error message was wrong: you returned the right error, but the handler ignores it, which was the long-standing bug the comments didn't describe very well. I opened #1326 fixing it, I'd prefer to wait for that before merging.

Copy link

Contributor

Author

hi-rustin commented 12 days ago

Thanks. That shows the error message was wrong: you returned the right error, but the handler ignores it, which was the long-standing bug the comments didn't describe very well. I opened #1326 fixing it, I'd prefer to wait for that before merging.

Got it. Besides this issue, is there anything else that needs to be changed in this PR?

Copy link

Member

jyn514 commented 12 days ago

@hi-rustin I don't plan to review this again until #1326 is merged. Feel free to ping me if I forget :)

hi-rustin

force-pushed the

hi-rustin:rustin-author

branch 3 times, most recently from 6f42d8f to 696ff69

12 days ago

@@ -90,21 +90,6 @@

</li>

</ul>

</div>

{# Show the crate authors #}

jyn514 11 days ago

Member

Could you show the owners instead?

hi-rustin 11 days ago

Author

Contributor

I am currently unable to check the topbar related pages because I am having problems with add-essential-files.
See: rust-lang/rustwide#41 (comment)

So I don't know if the whole topbar is currently displayed properly. Can you help check it? Or do you have any suggestions for solving the above problem.

Nemo157 11 days ago

Member

If you have working IPv6 then https://docs.rs.dev.nemo157.com/ is running this PR currently, looks like it's working to me.

hi-rustin 11 days ago

Author

Contributor

Looking at the code, I should have used the correct field.

hi-rustin 11 days ago

Author

Contributor

If you have working IPv6 then https://docs.rs.dev.nemo157.com/ is running this PR currently, looks like it's working to me.

Thanks!

hi-rustin 11 days ago

Author

Contributor

I am having problems with add-essential-files.
See: rust-lang/rustwide#41 (comment)

@jyn514 @Nemo157 Do you have any suggestions to solving this?

Copy link

Member

jyn514 left a comment

This is great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK