Skip to content

Conversation

oke-aditya
Copy link
Contributor

@oke-aditya oke-aditya commented May 11, 2022

image

The comment has unintentional side effect of appearing in css class over generated tables in html pages. Although it doesn't break anything, it could hurt sometime later.

cc @NicolasHug

@oke-aditya
Copy link
Contributor Author

To verify you can visit
https://pytorch.org/vision/main/models_new.html
right click
view page source and
see line number 382

While do the same on the current build you can do the same through Circle CI artifacts

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks @oke-aditya

@datumbox datumbox merged commit 03bb324 into pytorch:main May 12, 2022
@oke-aditya oke-aditya deleted the remove_css_class branch May 12, 2022 08:44
@NicolasHug
Copy link
Member

I had noticed this, but since it doesn't have any effect I kept it. I still think the comment is useful to provide context to the reader. Without it, we will definitely forget what this file does eventually. If we remove it, the docs will look terrible and we might not notive before it's too late.

@oke-aditya can you please try to see if we can add this comment on the line above? If not, I'd prefer referting this PR.

@oke-aditya
Copy link
Contributor Author

It's bit bad both the ways. Keeping it means we are injecting CSS which unnecassary (slowing the website) + say torch theme creates a CSS class that uses this. It would mean we are broken as we will override that CSS. I know the comment is crucial but having comment that injects code un intentionally is equally bad.

@NicolasHug
Copy link
Member

I'd really like to get this comment back one way or another. Do you mind trying adding it back in the file?

@oke-aditya
Copy link
Contributor Author

oke-aditya commented May 12, 2022

Lemme try adding it above so that it will not go inside CSS. Not sure if it would work

@NicolasHug
Copy link
Member

If it doesn't work, we can put back the comment as

#_Necessary_for_the_table_generated_by_autosummary_to_look_decent

This will never correspond to any css class and we'll be safe. Not amazing, but better than not comment at all.

@datumbox
Copy link
Contributor

I think it's possible to fix the HTML and keep the comment. See below:

The configuration file consists of sections, lead by a "[section]" header and followed by "name: value" entries, with continuations in the style of RFC 822; "name=value" is also accepted. Note that leading whitespace is removed from values. ... Lines beginning with "#" or ";" are ignored and may be used to provide comments.

From here.

facebook-github-bot pushed a commit that referenced this pull request May 24, 2022
Reviewed By: datumbox

Differential Revision: D36413366

fbshipit-source-id: 710068134dbf119675a3d50032b79bb59bb99f76
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants