potiuk commented on issue #34093: URL: https://github.com/apache/airflow/issues/34093#issuecomment-1705991452
Agree. It makes very little sense how it is defined. Especially the comment `Bare “.” will be replaced so you can set airflow*` is pretty misleaning in the context of your examples. I think - if we wanted to do it more intuitively (but keep the regexp compatibility) we should do it differently 1) Check if the pattern string ends with `.*`. If so use `glob` matching rather than regexp matching 2) Check if the string fully matches existing class (we can attempt to import the pattern and see if it works). 3) If none of the above, fallback to regexp matching 4) Update the documentation describing it (and treat it as a bugfix not breaking change). I believe this will strike the right balance between the intuitiveness of full-class specification, ability to glob-match when you want to exclude whole packages while keeping the power of regular expressions (both 1) and 2) are highly unlikely to be specified with the intention of being regular expressions). @kaxil @bolkedebruin - does it cover the intentions of the https://github.com/apache/airflow/pull/28829 ? I believe that was the intention. The `\,,.` replacement indeed does not properly match expressions that contain more than one top-level package and make the matching far less intuitive if you have hierarchical packages. -- 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]
