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

docs: describe how to use with non-serializable options #252

Merged
merged 2 commits into from
Oct 23, 2021

Conversation

simoneb
Copy link
Contributor

@simoneb simoneb commented Oct 22, 2021

Closes #242

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina requested a review from jsumners October 22, 2021 10:34
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Please hard wrap at ~80 characters to be consistent with the rest of the docs.

@simoneb
Copy link
Contributor Author

simoneb commented Oct 22, 2021

@jsumners the rest of the docs by no means comply with this arbitrary 80 character limit. In any case, if you really want to enforce that, the best way would be to get one of the automated checks to fail.

@jsumners
Copy link
Member

@jsumners the rest of the docs by no means comply with this arbitrary 80 character limit. In any case, if you really want to enforce that, the best way would be to get one of the automated checks to fail.

I expanded the first available expansion below the change in the GitHub diff and it showed the hard wrapping. I'd rather follow up with a "fix" on the rest of the docs later than continue the inconsistency here.

@simoneb
Copy link
Contributor Author

simoneb commented Oct 22, 2021

I'm sorry @jsumners, not trying to start an argument here but if you want certain standards in the codebase it's up to you to enforce them. You're using standardjs, which has no concept of maximum line length and you have nothing in place to enforce that.

You can easily see for yourself that the Readme file doesn't comply with any length limits and I have no easy way to format it that way simply because the tooling that's in the repository doesn't allow me to do that. So basically what you're asking me is that I manually go and break all lines to a maximum of 80 characters, which to be honest is not something which is fair to expect from any contributor unless the repository provides an easy way to do that.

@jsumners
Copy link
Member

Again, when I expand the diff in this PR, I can see that the rule is followed in at least the very next documentation section:

image

If you're using VSCode as your editor, you can can use https://marketplace.visualstudio.com/items?itemName=stkb.rewrap. Select the text to be wrapped and press ctrl+q (option+q). Otherwise, I haven't seen any modern editor that doesn't provide a right margin line that can be configured to an arbitrary width specifically to aid in this request.

I'm not requesting that you fix the rest of the docs, if they are indeed out of order. I am only asking that we not perpetuate the inconsistency.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@simoneb simoneb requested a review from jsumners October 22, 2021 16:35
Copy link
Member

@jsumners jsumners 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.

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.

messageFormat used as a function does not work with pino v7
3 participants