potiuk commented on PR #28367: URL: https://github.com/apache/airflow/pull/28367#issuecomment-1353867228
I think there is far too much of confusion here in general. I'd rather say having "mkdir" which behaves different than Python and POSIX standard is confusing. This was a decision made by Python creators and while we might argue with it, it has a good reason and it is explained in the docs of Python very clearly and explicitly https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir - so you might say a lot of users will be confused if aiflow's mkdir behaves differently. > If parents is true, any missing parents of this path are created as needed; they are created with the default permissions without taking mode into account (mimicking the POSIX mkdir -p command). I'd say having a different behaviour will only add to confusion (umask behaviour is sufficiently complex without it). I would also say that if we want to change behaviour of this in specific places and we have a good reason for it (do we?) undeprecating a method that have been deprecated > 2 years ago https://github.com/apache/airflow/pull/10117 (which would add even more confusion) is a bad idea. Now - we want to add a new mkdirs util method that not only creates parents by default (contrary to the system lib, but also does it differently). That's super-confusing, and there is even no attempt to explain it in the docs of hte newly added method. If we want to propose a change in behaviour of some pleces where currently Path.mkdir(parent=True) is used, then it should be a new feature added rather than silently changing it back. This ship has sailed. And father than adding similarly named function to utils, it should have different name and signature to make it clear it is not similar to mkdirs behaviour. But - this PR does not do that, so I am rather sceptical if we want to add it at all because it seems there is no intention to use it ? If we want to just add a new util file which is not used in airflow anyway why do we want to add it at all? If it's not used, there is no reason to add it. YAGNI. If it "just" needs to be used elsewhere, outside of Airflow (I guess AirBnB uses it), it should be added as a library there, not in Airflow. We ere **just now** discussing about being very explicit what Public Airflow Interface is https://github.com/apache/airflow/pull/28300 and the idea is there to be very explicit about what is and treat all the rest. Adding - effectively (after old one is deprecated for 2.5 years) a new util API which is unused makes very little sense to me. We only want to expose as external interface things that are "extremely" important to be exposed. We do not want others relying on random utils and small things in Airflow, because it makes it more difficult to maintain Airflow in the future. Are there any particular uses of it @pingzh in Airflow that you want to add now or soon (and if so - why it's not part of this PR)? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
