BasPH commented on issue #5383: [AIRFLOW-4737] Increase and document celery 
queue name limit
URL: https://github.com/apache/airflow/pull/5383#issuecomment-500216912
 
 
   Few things to consider here: migrations are also part of the Airflow 
codebase, so for consistency sake we like to apply the same conventions.
   
   Every function performs some sort of logic, so it's convenient for other 
people and even your future self to explain _what_ the function does, even if 
it's a simple one-liner. Typically business logic code is very hard to debug 
and understand - because others don't have the context you had while writing 
the code.
   
   IMO this migration is a perfect example of this - the number of chars is 
raised. If it ever turns out to be an issue and somebody else would look at the 
code, they don't see any explanation in the code stating why the number was 
raised and why 256 was chosen. Only after reading your JIRA issue do I 
understand why these choices were made, which are perfectly fine. 
Unfortunately, not everybody explains the what and why in their Git 
commit/PR/JIRA issue.
   
   Reading the Airflow code base, there are a lot of such examples of 
undocumented logic, so now that we are in the process of adding Pylint I prefer 
not to lower the boundaries just because it's inconvenient. If you'd write a 
DAG at your work and you're the only person maintaining the thing, I wouldn't 
really be bothered, but Airflow is software used and developed by many people 
around the world so I strongly prefer to have consistently documented code.
   
   Sorry for the wall of text, hope you understand 😃 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to