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

Improve docs Contributing to Pydantic section #441

Closed
pilosus opened this issue Mar 29, 2019 · 5 comments
Closed

Improve docs Contributing to Pydantic section #441

pilosus opened this issue Mar 29, 2019 · 5 comments

Comments

@pilosus
Copy link
Contributor

@pilosus pilosus commented Mar 29, 2019

Documentation's Contributing to Pydantic section says that basically once you've made your changes you run make format and then make. make invokes Makefile's all rule that includes tests with coverage report, linting and mypy recipes. Although you are safe to not breaking style guides and tests locally, you still can:

  1. Break rst files, so that sphinx will fail -- you'll find out once PR is open and Travis CI check is done (this is what I did in my very first PR)
  2. Slow down pydantic with the changes made -- you won't find out without make benchmark-all (great performance mentioned as one of the pydantic's advantages in Rationale section, so it's important, isn't it?)

I think there are a couple of ways to fix that:

  1. Add new Makefile rule called full-check:
.PHONY: full-check
full-check: testcov lint mypy external-mypy docs benchmark-all

Then add a short note to contributing section about it.

  1. Just add a note to contributing section to raise awareness about spinx build and performance check.

A small improvement to Benchmarks section could also be done. It probably should say that random module used in benchmarking gives non-deterministic results and also many other factors (e.g. other processes running by the OS when benchmarking) may also affect the benchmark results, so they probably should be taken with a grain of salt.

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Mar 29, 2019

I agree with full-check, except that benchmarking is harder than just running them and looking at the result (like one does with tests):

  1. Not all aspects of performance are covered by the benchmarks, eg. schema generation, other types
  2. We have to accept that sometimes extra features and checks reduce performance. That's just the way it is.
  3. Different benchmark data produces different results, we could have a deterministic set of benchmarks (maybe downloaded from s3 or similar) to avoid this but it's still not perfect.
  4. Perhaps because the benchmarks aren't long enough, you have to run them quite a few times before you get a firm result, unless you do something which as a dramatic effect which is rare.

There's also a few other problems like telling people to use single quotes not double.

Basically we should improve the "contributing" section in general, and tell people to run make docs as well as make.

@pilosus
Copy link
Contributor Author

@pilosus pilosus commented Mar 29, 2019

Basically we should improve the "contributing" section in general, and tell people to run make docs as well as make.

Should full-check also be implemented, except for benchmark-all recipe in it? If so, should all rule still be a DEFAULT_GOAL in Makefile, right?

I see the problems with benchmarking. It's indeed has to be run multiple times to get more or less acurate results. Maybe benchmarking and it's docs improvement should be a separate issue.

There's also a few other problems like telling people to use single quotes not double.

black still does not support signle quotes and most likely will never be supporting. I don't know of any other code formatters for Python that allow to fix quotes type only. But still using linters to detect double quotes reduces chances for contributors to use wrong quotes. Maybe something like flake8-quotes can help?

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Mar 29, 2019

I don't think make full-check is necessary, it's not much faster than typing make && make docs.

I agree on benchmarking being a separate issue.

Regarding single quotes & blank, I have a modified version of black that enforces single quotes (actually it only takes a one line change). However given the acrimony over the original issue, I'm hesitant to release it. For now I think it's fine just to document this.

@pilosus
Copy link
Contributor Author

@pilosus pilosus commented Mar 29, 2019

@samuelcolvin do you mind if I submit a PR with the improvements to contributing section we've discussed here?

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Mar 29, 2019

yes please, that would be great.

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

Successfully merging a pull request may close this issue.

None yet
2 participants