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

[JUJU-4553] Update README.md to simplify and better align with docs #1052

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

tmihoc
Copy link
Member

@tmihoc tmihoc commented Oct 20, 2023

This is part of a larger initiative to update and streamline the READMEs of all the main projects associated in the Juju universe.

@tmihoc tmihoc changed the title Update README.md to simplify and better align with docs [JUJU-4553] Update README.md to simplify and better align with docs Oct 20, 2023
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

Thanks! There's some nice bits in here, but also a lot where it's unclear why changes have been made, or where changes aren't all positive.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer added the docs Improvements or additions to documentation label Oct 23, 2023
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I agree with a lot of @tonyandrewmeyer's points here -- a can understand the desire to reduce the text and link it to more official places instead, but the big preformatted installation/usage example doesn't seem like an improvement.

@tmihoc I wonder if you could provide a bit more context on what we're aiming for here?

I wonder if it'd be good to brainstorm this with the 3 of us sitting together in Riga for 30-45 minutes?

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 6 to 10
> ✔️ `ops` is ([`available on PyPI`](https://pypi.org/project/ops/)).
>
> ⚠️ The latest version of `ops` requires Python 3.8 or above.
>
> 🥇 While charms can be written in any language, `ops` defines the latest standard. All new charms are encouraged to use it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call me a grumpy old man :-), but I don't think these particular emojis add any value. The check mark has little to do with PyPI, the warning sign isn't really a warning, and medal != standard. I think we could just make these bullet points. Suggestion (also note slight tweaks):

Suggested change
> ✔️ `ops` is ([`available on PyPI`](https://pypi.org/project/ops/)).
>
> ⚠️ The latest version of `ops` requires Python 3.8 or above.
>
> 🥇 While charms can be written in any language, `ops` defines the latest standard. All new charms are encouraged to use it.
- `ops` is [available on PyPI](https://pypi.org/project/ops/).
- The latest version of `ops` requires Python 3.8 or above.
- While charms can be written in any language, `ops` defines the latest standard. All new charms are encouraged to use Python with `ops`.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll drop the emojis. I'll also try to move the third bullet in the introduction, as it seems to fit better there. (And some of the content there seems unnecessary.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to call me a grumpy old man too 😂, but I agree about the emojis - we still have the right-hand-pointing one in the table at the top. I'm not sure why that row gets/needs an emoji?

I don't feel super strongly about it (and in all seriousness, maybe I am showing my age) so if it has value then ignore this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

we still have the right-hand-pointing one in the table at the top

I wanted the table to act like a map to the Juju universe. I'm using it for the Juju and Charmcraft READMEs too. The right-hand-pointing emoji is supposed to be the "You are here" mark. PS I'm hoping some day we can replace it with a nice visual to the same effect -- ideally a visual with clickable links, just like in the table.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
Comment on lines 124 to 80
...
import ops.testing
...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
...
import ops.testing
...
import ops.testing

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the dots to indicate a partial quote. Happy to drop the first set though.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I really like this now, thanks! It's good to have the test example, and I think linking to the setup/cleanup works much better than inlining it.

I've left a suggestion for the charm/directory name (I would like to improve this), and a few other very minor comments (take them or leave them).

I'll also get @tonyandrewmeyer's feedback on the latest iteration.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

> See [Charm SDK | Set up your development environment automatically > Clean up](https://juju.is/docs/sdk/dev-setup#heading--automatic-set-up-an-ubuntu-charm-dev-vm-with-multipass).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here about the link being a mouthful, and probably not needing the full tree. Suggestion:

Suggested change
> See [Charm SDK | Set up your development environment automatically > Clean up](https://juju.is/docs/sdk/dev-setup#heading--automatic-set-up-an-ubuntu-charm-dev-vm-with-multipass).
> See [Charm SDK > Clean up](https://juju.is/docs/sdk/dev-setup#heading--automatic-set-up-an-ubuntu-charm-dev-vm-with-multipass).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
# Learn more about statuses in the SDK docs:
# https://juju.is/docs/sdk/constructs#heading--statuses
self.unit.status = ops.ActiveStatus()
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor

I really like this now, thanks! It's good to have the test example, and I think linking to the setup/cleanup works much better than inlining it.

I've left a suggestion for the charm/directory name (I would like to improve this), and a few other very minor comments (take them or leave them).

I like this too, thanks! I also left a few formatting nits, +1'd the charm name, and had some take-or-leave-them minor comments.

Other than the example charm name, the one thing that I feel needs to get resolved is the ops/__init__.py alignment. I'm happy if the syncing is removed, but if so then the PR should remove the line saying that they should stay in sync.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for seeing this through!

@benhoyt benhoyt merged commit c2f8216 into canonical:main Oct 26, 2023
18 of 20 checks passed
@benhoyt
Copy link
Collaborator

benhoyt commented Oct 26, 2023

Merged -- thanks @tmihoc! Not related to this PR, but linting is broken right now due to Python 3.12 deprecations. Tony is going to fix that in a follow-up PR.

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

Successfully merging this pull request may close these issues.

4 participants