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

doc(Tutorial): Add explanation for app config (#4137) #4212

Closed
wants to merge 1 commit into from

Conversation

oktak
Copy link

@oktak oktak commented Oct 17, 2019

What do these changes do?

Documentation on Tutorial section, about explanation on why aiohttp chooses to store configurations in one place(the main app).

Are there changes in behavior for the user?

No.

Related issue number

#4137

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@asvetlov asvetlov added the invalid This doesn't seem right label Oct 18, 2019
@asvetlov
Copy link
Member

@Arfey please take a look.
I suspect that we should close the PR with 'invalid' label.
Yet another catcher of Hacktoberfest T-Shirts

@Arfey
Copy link
Member

Arfey commented Oct 18, 2019

@asvetlov i believe that we'll make with that MR something helpful 😀
I think this is the first place where they quote me 😅

cleaning up. The discussion is ongoing ` (issue
`#3876 <https://github.com/aio-libs/aiohttp/issues/3876>`_).
`@mrasband <https://github.com/mrasband>`_ suggested to push subapplication
configurations to one of the key in the ``app`` mapping.
Copy link
Member

Choose a reason for hiding this comment

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

@oktak thx for your contribution.

I'm not sure that it's what actually we need. But we can improve that.

  1. We don't need to create a new stage of documentation. Instead of that the better way will be add this information in place where is this used for the first time. I found this place.
  2. We can use Note block for that.
  3. Save all configuration and connection it's just convention (but best practice). We need so easy and shot explain why we need to use this approach and what benefits we get.

Copy link
Member

Choose a reason for hiding this comment

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

@asvetlov may be u will put here a list of some things which u want to show in this part of documentation.

As example:

  • this is helpful when you want to share your setting between subapp because ...

Of course, if you'll a free time.

Copy link
Member

Choose a reason for hiding this comment

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

You are the author of #4137, not me.
I appreciate any docs improvement and happy to review but I have no such list of proposals in my memory.

@oktak
Copy link
Author

oktak commented Oct 19, 2019

@asvetlov , I disagree that you simply close the PR with 'invalid' label without stating more specific requirement that like @Arfey does.

True that I am aiming for T-shirt. But, I don't think my pull request is totally useless. I am actually want to help to tackle the issue #4137 . And I did a few research, and discuss with the issue opener. I tried to guess what end-product actually they want. I clone the repo and, build the docs, add the page.

If anyone does not welcome Hacktoberfest joiner, please do not add Hacktoberfest labels in all their issues.

Aiohttp is a nice project, I love to use it. So I choose to help.

@asvetlov
Copy link
Member

@oktak you've created tutorial/index.rst
If you open http://docs.aiohttp.org/en/stable/web.html you'll see that aiohttp tutorial already exists, it is hosted on separate https://github.com/aio-libs/aiohttp-demos repository

Your PR looks so... incomplete; I've thought that it has no intention to be merged.

@oktak
Copy link
Author

oktak commented Oct 20, 2019

@asvetlov Thank you for your clarification. I am clearer now.
I know there exists the tutorial page, and tried re-confirm it:
#4137 (comment)

It turns out I'd better find my way to contribute on the separated https://github.com/aio-libs/aiohttp-demos repository, rather than this main repository. With the review comments, I think I could make more complete PR.

I suggest your team also close issue #4137 or add an invalid label to it as well, and then open new issue on demos repository.

@oktak oktak closed this Oct 20, 2019
@asvetlov asvetlov removed the invalid This doesn't seem right label Oct 20, 2019
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