You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/07 02:47:30 UTC
[GitHub] [airflow] dstandish opened a new pull request, #28182: Better error message for bad stat name
dstandish opened a new pull request, #28182:
URL: https://github.com/apache/airflow/pull/28182
Old error was a bit silly, printing out the repr of the set.
```
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/airflow/stats.py", line 232, in wrapper
stat = handler_stat_name_func(stat)
File "/usr/local/lib/python3.9/site-packages/airflow/stats.py", line 201, in stat_name_default_handler
raise InvalidStatsNameException(
airflow.exceptions.InvalidStatsNameException: The stat name (dag_processing.last_duration.intldldat-k1wm-full-dev-k1-local-tables-wfoptimizedlayer - Copy) has to be composed with characters in
{'j', 'Q', 'G', 'v', 'l', 'y', '4', 'N', 'f', 'U', 'F', 'P', 'w', 'V', '.', '-', 't', 'T', 'p', '1', 'D', 'd', 'I', 'C', 'k', 'A', 'a', 'q', 'c', 'i', 'o', 'e', 'u', 'X', 'O', '0', 'g', '6', '5', 's', 'Y', 'n', 'E', 'H', 'Z', 'L', 'h', 'b', 'z', '3', 'x', 'B', 'K', 'S', 'W', '2', '_', 'r', 'm', 'J', '9', '7', 'R', '8', 'M'}.
```
Using regex is more compact.
```
Traceback (most recent call last):
File "/usr/local/opt/python@3.8/Frameworks/Python.framework/Versions/3.8/lib/python3.8/code.py", line 90, in runcode
exec(code, self.locals)
File "<input>", line 1, in <module>
File "/Users/dstandish/code/airflow/airflow/stats.py", line 196, in stat_name_default_handler
raise InvalidStatsNameException(f"The stat name ({stat_name}) must match regex {pattern}.")
airflow.exceptions.InvalidStatsNameException: The stat name (abc abc) must match regex ^[A-Za-z0-9_.-]+$.
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1340536106
you don't think that, given that the work has already been done, it's more positive than negative to remove this constant, which ultimately is not really needed and is somewhat vague?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #28182: Better error message for bad stat name
URL: https://github.com/apache/airflow/pull/28182
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1356907529
And just to explain a bit - I realised closing the issue could have sounded a bit rude. But it was not really and in my mind it was pretty clear at that time (and this is how I interpreted the title of the PR and the "oops" + opening another issue by TP. But I really thought the excessive code - especially the `getattr` to provide backwards compatibility was totally not worth it. Effectively the trade-off was:
a)const definition stays
vs.
b) importing regexp library + adding getattr method in which the same constant was effectively repeated, adding new method where not only logic for gettattr + adding tests if regexp works + message that required the user to parse the regexp
The title of the PR was "better message" which was not too much better (telling regexp in a user message is a sign that we expect the user can parse the regexp, which is kinda non-human.
Simply speaking the change from TP looked so much more doing what your PR title suggested - "WAY better message".
There could be other reasons why we could get rid of the constant, but I think just "better user message" is not one of those reasons.
Sorry if you took that as being rude or smth like that. It was just perfectly logical choice for me.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1356876053
> you don't think that, given that the work has already been done, it's more positive than negative to remove this constant, which ultimately is not really needed and is somewhat vague?
Sorry missed that one as I did not realise it was for me, but I am catching up now after moving to a different country :).
I just think it's a lot of code to add for such a simple thing. The way it is defined now explains it in the code pretty well (for maintainers) and the error message is very clear about it (for users). It serves the purpose pretty well. This is one line of code change rather than 40 lines of code added 10 removed here (net + 30 lines)
That was my primary thinking by choosing "better". I usually do not think that something is "done" is a reason for its own to classify it as "better". Quite often my own "done code" is reworked, simplified and removed - sometime during the same PR, sometimes months or years later. And I am completely indifferent to the fact that it's "done". Because "done" is usually few percent of the effort needed for lifecycle of the code. I believe code is usually a liability that has to be maintained rather than asset. If it can be done in far less code - why not remove something that is done.
If something serves the same purpose and is clear with far less code or no code at al - then for me it is "better".
I actually love to remove my own code when possible while achieving same result. For me that means one less thing I have to maintain in the future (and that extrapolates to all the maintainers of Airflow).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1340357748
Oops #28181
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1340388707
> Oops #28181
lol
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28182: Better error message for bad stat name
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28182:
URL: https://github.com/apache/airflow/pull/28182#issuecomment-1340523131
Prefer @uranusjr if only from review POV :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org