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.

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

Removed dict annotation from GeoAxes.gridlines #2386

Merged
merged 1 commit into from
May 17, 2024

Conversation

BhavyeMathur
Copy link
Contributor

Rationale

Just something that had been bothering me for a bit. The **kwargs parameter in GeoAxes.gridlines is itself a dict, but shouldn't be annotated as dict because that implies that it accepts only dictionary keyword arguments. For example, the following code is highlighted with a warning:

ax.gridlines(..., linewidth=0.15)  # syntax highlighter expects linewidth to be a dictionary

Implications

Better syntax highlighting. Warning is no longer displayed for code that is correct.

@CLAassistant
Copy link

CLAassistant commented May 17, 2024

CLA assistant check
All committers have signed the CLA.

@BhavyeMathur
Copy link
Contributor Author

BhavyeMathur commented May 17, 2024

Some of the tests have failed due to 404 errors. I don't think those are related to my singular change however.

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Hi @BhavyeMathur, thanks for this.

Cartopy follows the numpydoc style and according to the guide, including the dict type is correct. However, I also looked up a few examples in numpy itself, and only one of them show a type for **kwargs, which is any.

I cannot think of a scenario where stating this is a dict is useful information for the user, so if it causes a problem, let's take it out. Out of interest which editor do you use?

Although the doc build is broken, the relevant docstring renders fine.

@rcomer rcomer added this to the Next Release milestone May 17, 2024
@rcomer rcomer merged commit 6adeeaa into SciTools:main May 17, 2024
9 of 20 checks passed
@BhavyeMathur
Copy link
Contributor Author

BhavyeMathur commented May 17, 2024

Hi, thanks for merging the pull request.

Interesting, I wonder why NumPy recommends those hints for *args and **kwargs. Annotating it Any should work without problems too, if that's something you want to pursue. Noticed the issue while using JetBrains PyCharm and DataSpell, but looks good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants