Skip to content

Conversation

hashlash
Copy link
Contributor

@hashlash hashlash commented Mar 20, 2020

@hashlash
Copy link
Contributor Author

After doing some research I've found some alternative for async reserved word used by some open-source projects:

From those alternatives, I think the most appropriate is asynchronous. The only drawback I can think of is its verbosity (and word length).

  • async_: I think it's more appropriate on local variables

  • async_val: consist of two words and what we want is not just any regular 'value'

  • async_safety (proposed in here): same as async_val, and the word length is similar with asynchronous

  • asgi (from this comment): argument from @andrewgodwin:

    ... but asgi is sort of misleading, this happens under synchronous environments too

@hashlash hashlash requested a review from andrewgodwin March 21, 2020 06:41
@adamchainz
Copy link
Member

I'm slightly against async_safety since the tag should be general purpose enough for use with multiple checks. There could be other async related checks that aren't for "safety" purposes necessarily, but performance, library compatibility etc. But if it's the best then we can go for it.

I'm also slightly against asynchronous since it's an adjective not a noun, and all the other tags are nouns. The correct noun would be asychronicity but I just struggled to spell that even.

I'm still fine with on async_. The string for display can still be without the trailing underscore. We can understand it as short for asynchronicity or async def or whatever.

Fine with async_val, or other combinations like async_usage, async_compatibility, etc.

Don't want to bikeshed.

Copy link
Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Great work @hashlash

@hashlash
Copy link
Contributor Author

I just realized that the models tagged system check module is named model_checks, so I rename the module to async_checks

Copy link
Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

I've come back and reviewed this again, looking good. I've made a few more edits. I would push them to your branch so you don't have to do anything but since DEP009 github won't let anyone but fellows do that.

Here's a patch - can you apply it please? :)

patch.txt

@hashlash
Copy link
Contributor Author

Thanks for the guidance @adamchainz! Will do it right away

Copy link
Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Just realized my patch file was in reverse, sorry!

Looking good apart from a couple things missed because patch files aren't perfect.

@felixxm felixxm changed the title Fixed #31380 -- Prohibited DJANGO_ALLOW_ASYNC_UNSAFE on deploy check Fixed #31380 -- Added deployment system check for DJANGO_ALLOW_ASYNC_UNSAFE environment variable. Apr 1, 2020
@felixxm
Copy link
Member

felixxm commented Apr 1, 2020

@hashlash Thanks 👍 Welcome aboard ⛵

I pushed minor edits.

@felixxm felixxm removed the request for review from andrewgodwin April 1, 2020 10:58
@adamchainz
Copy link
Member

:shipit:

@felixxm felixxm merged commit 4a6f2b6 into django:master Apr 1, 2020
@hashlash
Copy link
Contributor Author

hashlash commented Apr 1, 2020

@hashlash Thanks Welcome aboard

You're welcome! It's nice to be able to contribute 😄

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