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

Update and simplify README #2774

Merged
merged 23 commits into from
Jan 5, 2023
Merged

Update and simplify README #2774

merged 23 commits into from
Jan 5, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Dec 28, 2022

I updated the README.md. Furthermore, I really like READMEs which are concise, similar to https://github.com/pandas-dev/pandas, i.e. a README which summarizes what the library does, a fully contained example that can be copy-pasted, simple installation instructions, and a reference to the documentation. This is where users can then go to learn more. Afterwards, some additional content for developers. In my opinion, all the rest should be in the documentation so it's easier to find.

Reasoning for the changes is usually in the commit messages but let me know if anything is unclear!

Included content from #1122 in CONTRIBUTING.md

Is the Google Altair Group still something that should be actively recommended for asking questions or only StackOverflow? I think ideally there is one preferred place to ask questions (StackOverflow) and one place to report feature requests and bugs (GH issues). But as the Google Group already has quite some content, maybe we want to keep the reference to it and I could also add it in the Getting Help page in the documentation?

@binste binste marked this pull request as ready for review December 28, 2022 15:02
@mattijn
Copy link
Contributor

mattijn commented Dec 29, 2022

Thanks for the work here. I'm trying to understand the changes you made to the file docbuild.yml here:

https://github.com/altair-viz/altair/pull/2774/files#diff-5f636d76e1060c18bf31565648228e016c5511b2cceed87d82af54c17d4c3f21

Can you explain a bit why you think that is necessary? Ah, maybe because you updated the requirements.txt as well.

I think we still need altair_saver for the official docs, since vl-convert cannot create the png-thumbnail of isotype charts:

vl-convert:
image
altair_saver:
image

@mattijn
Copy link
Contributor

mattijn commented Dec 29, 2022

There is also an Altair channel with >150 members in the Vega slack group. But it is not active. I think sticking to SO and GH is fine.

…er is required for deployment as one thumbnail cannot be generated otherwise
@binste
Copy link
Contributor Author

binste commented Dec 30, 2022

I thought it's nice if we can switch to recommending vl-convert in CONTRIBUTING.md as it greatly simplifies the setup needed for building the docs. For example to get altair_saver working in a Debian (bullseye) Docker container running on an Apple M2 chip, I had to install the following additional dependencies on top of what is described in the altair_saver docs:

sudo apt-get install build-essential libcairo2-dev libpango1.0-dev libjpeg-dev libgif-dev librsvg2-dev

Consequently I also adapted it in docbuild.yml but I wasn't aware of the emoji limitation (turns out it's already documented at https://github.com/vega/vl-convert#emoji-support-in-png-export), thanks for noticing!

I reverted the change in docbuild.yml and requirements.txt and clarified in RELEASING.md and CONTRIBUTING.md that vl-convert-python is the easier way to quickly build the docs but for releases one needs to install altair_saver and uninstall vl-convert-python as the later is used by default.

@mattijn
Copy link
Contributor

mattijn commented Dec 30, 2022

Yes, I agree. Everything outside building the docs should recommend using vl-convert. I think it’s possible to define/overrule an engine to be used to export the pngs. It would be a bit unpleasant if others have to delete packages for setting this.

@mattijn
Copy link
Contributor

mattijn commented Dec 30, 2022

And with, building the docs I mean, the official one within GitHub actions. For contributors it really is much easier to use vl-convert. Therefor I’m also not against including it in the requirements.txt. If that means a few more steps should be included to make the GitHub action workflow correctly work with altair_saver, then that’s fine by me.

Copy link
Contributor

@mattijn mattijn left a comment

Choose a reason for hiding this comment

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

A few more suggestions

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
@mattijn
Copy link
Contributor

mattijn commented Dec 30, 2022

I think it’s possible to define/overrule an engine to be used to export the pngs. It would be a bit unpleasant if others have to delete packages for setting this.

This PR is related to this: #2784

@binste
Copy link
Contributor Author

binste commented Dec 30, 2022

Thanks @mattijn! I agree, it's great if its easier for new contributors so I readded vl-convert-python to doc/requirements.txt and clarified in CONTRIBUTING.md when it needs to be replaced with altair_saver. In the github workflow, we can simply uninstall vl-convert-python.

With this, it's only a bit cumbersome when releasing a new version but at least it's well documented. I'm not sure how it currently works with access rights to pypi and this repo, but a more reliable way to publish new versions and update the documentation would probably be with github workflows, e.g. see the Pandas repo wheels.yml and docbuild-and-upload.yml.

RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
@mattijn
Copy link
Contributor

mattijn commented Dec 30, 2022

I agree that streamlining the publication of releases to PyPi and Conda using Github Actions would be an improvement as it simplifies maintenance. Currently only Jake has the permissions to do so, perhaps creating a new issue to track this would be a good idea. It's likely that setting this up correctly will require some discussion to implement it properly.

Co-authored-by: Mattijn van Hoek <[email protected]>
@binste
Copy link
Contributor Author

binste commented Dec 31, 2022

Sounds good, I'll create an issue with some first inputs for discussion. The GH actions might be a great way to share the credentials for pypi and conda between maintainers thereby reducing the single-person dependency risk on Jake.

Co-authored-by: Mattijn van Hoek <[email protected]>
Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! A few minor comments

CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/getting_started/getting_help.rst Outdated Show resolved Hide resolved
doc/getting_started/getting_help.rst Outdated Show resolved Hide resolved
doc/getting_started/overview.rst Show resolved Hide resolved
doc/getting_started/project_philosophy.rst Show resolved Hide resolved
@joelostblom joelostblom merged commit 8a71d2e into vega:master Jan 5, 2023
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.

4 participants