Skip to content

Conversation

@timhoffm
Copy link
Member

While not visible in the cheatsheet itself, we should still adhere to common best practices, to be a role model for potential contributors.

While not visible in the cheatsheet itself, we should still adhere
to common best practices, to be a role model for potential contributors.
ax.step(X, Y, color="C1", linewidth=0.75)
ax.set_xlim(0, 8), ax.set_xticks(np.arange(1,8))
ax.set_ylim(0, 8), ax.set_yticks(np.arange(1,8))
ax.set_xlim(0, 8), ax.set_xticks(np.arange(1, 8))
Copy link
Member

Choose a reason for hiding this comment

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

Could this use ax.set now? At the very least, if we're going to stuff multiple statements in one line, we should use ;, not , to fake it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we have that kind of code in our plot-types gallery. I thought I don't make too many changes at once to keep reviewing simpler.

@jimustafa
Copy link
Contributor

Would anyone object to applying Black code formatting? If not, I can open a PR. Whitespace formatting and PEP-8 compliance could be enforced through pre-commit checks.

@timhoffm
Copy link
Member Author

We intentionally don't use black in the matplotlib core codebase. And personally, I'm not a big fan of it, but it would be ok for me in this repo.

@jklymak
Copy link
Member

jklymak commented Nov 18, 2021

I'm fine with this as well - black seems to fall on its face for complicated code, but I think most of these scripts are simple enough that it will work well. OTOH, I don't use black day-to-day, so would I need to add a pre-commit hook or something?

@jklymak
Copy link
Member

jklymak commented Nov 18, 2021

a) I'm fine with these,
b) I'm lazy, but it would be nice to know none of these figures changed due to a typo. This gets into the same problem the main library has, but I wonder if we should add image comparison tests so we can see if code changes change images, and so we can track those changes over time.

@jimustafa
Copy link
Contributor

Actually, I also do not care for it much, but it seems to be the best game in town. Adds a bit too much whitespace around maths expressions, IMHO. Maybe just checking for PEP-8 violations is sufficient.

I started looking at matplotlib/pytest-mpl; @jklymak, this could be a way to do regression testing on the figures and cheatsheets. Ideally, not every tiny change is tracked, and comparisons can be made between the current and baseline versions, maybe with the baseline tracked in a separate branch and replaced on each successful build (like the gh-pages branch). This might miss a small drift, but could save on bloating the repo.

@jklymak
Copy link
Member

jklymak commented Nov 18, 2021

Cool - I hope this wouldn't end up too bloated as I don't think there is much call for changing the images often. OTOH they may change as Matplotlib changes, which may end up being a pain...

@jklymak
Copy link
Member

jklymak commented Nov 18, 2021

BTW, I'd just vote for flake8 as that is less invasive than black, but that is just my (relative) familiarity with flake8 speaking

@jimustafa
Copy link
Contributor

I will follow up on this PR with another that adds flake8 and applies most of the fixes for PEP-8 compliance.

@rougier
Copy link
Member

rougier commented Nov 22, 2021

I would also prefer pep-8 over black because I find black a bit too intrusive.

@rougier rougier merged commit 798ee83 into matplotlib:master Nov 23, 2021
@timhoffm timhoffm deleted the formatting branch November 23, 2021 07:41
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.

5 participants