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: fix very minor typo #894

Merged
merged 1 commit into from Dec 29, 2021
Merged

docs: fix very minor typo #894

merged 1 commit into from Dec 29, 2021

Conversation

hkclark
Copy link
Contributor

@hkclark hkclark commented Dec 28, 2021

Summary

Very minor typo in docs (and a few other questions/points).

Thank you for the great release with 21.3.0... I have already updated a big project I'm working on to use it. :-)

I found this minor typo in the docs.

Also, a couple of other points questions if you don't mind -- I normally wouldn't bring these up in a PR, but since I'm not sure of another place to discuss this, I thought this was better than nothing:

1)

This is really minor, but the phrase "vastly popular" in docs/names.rst feels a bit awkward to me. I would expect to see "vastly more popular" if the intent is "people are abandoning the previous way in droves" or "very popular" if the intent is "people seem to really like the new way". I wasn't sure the intended meaning, so I didn't include this in the PR and just wanted to submit it for consideration.

2)

This point is probably very controversial (and probably discussed somewhere else I'm not aware of), but I wanted to ask anyway. :-) Hopefully that's OK. :-) I was recently reviewing some code that used @define and it caused me to pause and think "what?!?" and then I remembered the "next gen" attrs stuff and it made sense. But to people not familiar with attrs and the modern API, it might leave them scratching their head for a bit. Yeah, I know at the top you would have from attrs import define and that should clue them in. But wouldn't it be more explicit and clear to do this?

import attrs

@attrs.define
class Coordinates2X:
    x: int
    y: int = attrs.field(
        default=attrs.Factory(
            takes_self=True,
            factory=lambda self: self.x * 2
        )
    )

I know people are free to do it either way and that changing the "recommended way" that's shown in the examples probably isn't something you guys want to change at this point, but I wanted to throw it out there since I think it would make it a bit more accessible for people new to attrs.

Pull Request Check List

  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

@hynek
Copy link
Member

hynek commented Dec 29, 2021

Summary

Very minor typo in docs (and a few other questions/points).

Thank you for the great release with 21.3.0... I have already updated a big project I'm working on to use it. :-)

I found this minor typo in the docs.

Also, a couple of other points questions if you don't mind -- I normally wouldn't bring these up in a PR, but since I'm not sure of another place to discuss this, I thought this was better than nothing:

1)

This is really minor, but the phrase "vastly popular" in docs/names.rst feels a bit awkward to me. I would expect to see "vastly more popular" if the intent is "people are abandoning the previous way in droves" or "very popular" if the intent is "people seem to really like the new way". I wasn't sure the intended meaning, so I didn't include this in the PR and just wanted to submit it for consideration.

Yeah, you're probably right. I'll change it to "very".

2)

This point is probably very controversial (and probably discussed somewhere else I'm not aware of), but I wanted to ask anyway. :-) Hopefully that's OK. :-) I was recently reviewing some code that used @define and it caused me to pause and think "what?!?" and then I remembered the "next gen" attrs stuff and it made sense. But to people not familiar with attrs and the modern API, it might leave them scratching their head for a bit. Yeah, I know at the top you would have from attrs import define and that should clue them in. But wouldn't it be more explicit and clear to do this?

import attrs

@attrs.define
class Coordinates2X:
    x: int
    y: int = attrs.field(
        default=attrs.Factory(
            takes_self=True,
            factory=lambda self: self.x * 2
        )
    )

I know people are free to do it either way and that changing the "recommended way" that's shown in the examples probably isn't something you guys want to change at this point, but I wanted to throw it out there since I think it would make it a bit more accessible for people new to attrs.

I personally use the explicit namespace in my own code, but that's partly also courtesy of "porting" using search-and-replace. One of the nice things of the new namespace is that it works both ways, the reason why it's using the imports is that i was being whiney when Tin suggested changing the docs to NG about the extra work when i introduce import attrs.

I guess we could change the README back, but otherwise the fully qualified are a tad too long to use them everywhere I think.

@hynek hynek enabled auto-merge (squash) December 29, 2021 06:42
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

thanks!

@hynek hynek merged commit 7695908 into python-attrs:main Dec 29, 2021
hynek added a commit that referenced this pull request Dec 29, 2021
@hkclark
Copy link
Contributor Author

hkclark commented Dec 29, 2021

@hynek One thought re the explicit namespace question might be to show/mention both ways once or twice in the docs just so people who like to copy paste exact examples at least see both ways and don't think it has to be all one way or the other or is considered "bad style." For example, between these two lines in "attrs by Example":

So in other words: attrs is useful even without actual attributes!
But you’ll usually want some data on your classes, so let’s add some:

it could say something like this:

One note: you can obviously also format your code like this:

import attrs
@attrs.define
class Empty:
   pass

And some of the maintainers of attrs actually prefer this style where you use the explicit attrs namespace
every time you reference an attrs feature -- it makes it very clear where attrs-specific features are being
invoked and can make the coder easier to read by people who are less familiar with attrs. However, for
the sake of brevity, the documentation will continue to use the shorter format shown above and below.

@hkclark
Copy link
Contributor Author

hkclark commented Dec 29, 2021

@hynek
PS - Again, the idea above is just a thought/suggestion for your consideration. However, I have worked on projects where people took the "style" from the docs and became quite dogmatic that it had to be used in that one style or it was "wrong."

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.

None yet

3 participants