Hello! This PR adds the tags for the simple scatter demo. I tagged a few more examples during the sprint. I wanted to start with just one to see how it goes.

Link to the scatter demo:
https://matplotlib.org/devdocs/gallery/animation/simple_scatter.html

Related:#27235

PR checklist

"closes #0000" is in the body of the PR description tolink the related issue
[N/A] new and changed code istested
[N/A]Plotting related features are demonstrated in anexample
[N/A]New Features andAPI Changes are noted with adirective and release note
Documentation complies withgeneral anddocstring guidelines

story645 added the Documentation: tagstagging examples + proposing new tags label

Nov 9, 2023

story645 reviewed

Nov 9, 2023

View reviewed changes

galleries/examples/animation/simple_scatter.py Outdated


		# %%
		#
		# .. tags:: component: animation, component: scatter, purpose: reference, level: intermediate

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 scatter is plot_type: scatter?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Corrected. thank you!

github-actionsbot reviewed

Nov 9, 2023

View reviewed changes

Copy link

github-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

karthikkurella approved these changes

Nov 13, 2023

View reviewed changes

story645 reviewed

Nov 13, 2023

View reviewed changes

galleries/examples/animation/simple_scatter.py Outdated

Comment on lines 37 to 38

		# .. tags:: component: animation, plot type: scatter,
		# .. tags:: purpose: reference, level: intermediate

Copy link

Member

story645Nov 13, 2023

Choose a reason for hiding this comment

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

Suggested change

	# .. tags:: component: animation, plot type: scatter,
	# .. tags:: purpose: reference, level: intermediate
	# .. tags:: component: animation, plot type: scatter, purpose: reference, level: intermediate

tags can all be on one line.

Copy link

Member

ksundenNov 15, 2023

Choose a reason for hiding this comment

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

I think this edit should be made for two reasons:

the rendering makes it so that there are twoTags: sections in the html
the trailing comma makes it treat"" as a tag which displays oddly and I think is causing the doc build failure.

That said, putting it on one line does break linting due to line length, does the tag directive have a good way of dealing with that/splitting across lines in the same directive?

Copy link

Member

story645Nov 15, 2023

Choose a reason for hiding this comment

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

So I think question for@melissawm if the following would be an ok ask

..tags::    tag1, ......., tagn   tagn+1, ....., tagn+8

Copy link

ContributorAuthor

gougouasmiNov 16, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing the commits and the suggestion! I made the change suggested above

Copy link

Member

melissawmNov 16, 2023

Choose a reason for hiding this comment

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

I think that is a reasonable suggestion. I will open an issue on sphinx-tags and try to get to it. For now can we ignore the linting on this line/file?

story645 approved these changes

Nov 13, 2023

View reviewed changes

Copy link

Member

story645 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Stylistic nitpick, but otherwise looks good.
ETA: Basically, I'll leave this open for a bit so you can choose whether to make the change I suggested, but I think it's okay to merge w/o the change.

Copy link

Member

ksunden commentedDec 20, 2023

Cycling to rerun doc test

ksunden closed this

Dec 20, 2023

ksunden reopened this

Dec 20, 2023

Copy link

Member

story645 commentedDec 21, 2023•
edited
Loading

@ksunden so I've got multiline content tags implemented, it's just waiting on reviews and mergingmelissawm/sphinx-tags#84 which should be good to go aftermelissawm/sphinx-tags#89

But doc tests will fail w/o those PRs in.

story645 reviewed

Dec 25, 2023

View reviewed changes

galleries/examples/animation/simple_scatter.py OutdatedShow resolvedHide resolved

Copy link

Member

story645 commentedDec 27, 2023

@gougouasmi did you want to handle the rest of your tags? I left them out of my batch PRs in case you did.

Copy link

ContributorAuthor

gougouasmi commentedDec 28, 2023

Hello@story645 ! I am a bit lost about what to do from here. Are we waiting on two other PRs?melissawm/sphinx-tags#84 andmelissawm/sphinx-tags#89

Copy link

Member

story645 commentedDec 28, 2023

Are we waiting on two other PRs?melissawm/sphinx-tags#84 andmelissawm/sphinx-tags#89

Kinda - those implement support for putting the tags in the body of the directive:

..tag::   t1, t2, etc

But tags on one line work perfectly well. Basically if you're still interested you can put in the PR even if it'll be a bit before it can get merged - just put the tags on one line to test that it works.