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/07/15 16:23:02 UTC
[GitHub] [airflow] pierrejeambrun opened a new pull request, #25098: Upgrade API file to typescript
pierrejeambrun opened a new pull request, #25098:
URL: https://github.com/apache/airflow/pull/25098
Move files under `js/api` to typescript.
related: https://github.com/apache/airflow/issues/24350
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r956446810
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
By the time, `getMetaValue` return type changed to string. This is not necessary anymore.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just a little concerned that the implementation for casting all values (potentially `any` type) to `string` might not be trivial to keep clean.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just afraid that the implementation for casting all values (potentially any type) to string will be hard to keep clean.
--
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] bbovenzi commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923193621
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
I wonder if it's worth it to make a URLSearchParams wrapper to convert everything to strings automatically.
--
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] bbovenzi commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923190466
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
Yeah, setting it to an empty string just kicks the error down the road. I don't know when these are not required, so I don't think we need an extra param. `getMetaValue()` should raise an exception and return empty string by default instead of coding it each time.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just a little concerned that the implementation for casting all values (potentially any type) might not be trivial to keep clean.
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just a little concerned that the implementation for casting all values (potentially any type) to string might not be trivial to keep clean.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea to avoid repeating multiple `toString` everywhere. I can think of other places where this will be necessary, for instance pagination params (pageNumber, pageSize etc.).
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r922656153
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
You are right, most of the `getMetaValue` are required, defaulting to an empty string is not great, and this pattern get duplicated everywhere. Raising an exception seems fair.
We could parametrize this with something like:
```
function getMetaValue(name: string, required: boolean = true)
```
--
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] pierrejeambrun commented on pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #25098:
URL: https://github.com/apache/airflow/pull/25098#issuecomment-1193358923
@bbovenzi Of course the branch is rebased :)
--
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] mik-laj commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
mik-laj commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r922639815
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
Should we raise exception when this value is missing? This value seems to be required.
--
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 #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25098:
URL: https://github.com/apache/airflow/pull/25098#issuecomment-1194141001
Rebased to account for "Werkzeug" fix
--
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] bbovenzi merged pull request #25098: Upgrade API files to typescript
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #25098:
URL: https://github.com/apache/airflow/pull/25098
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r922656153
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
You are right, most of the `getMetaValue` are required, defaulting to an empty string is not great. We could update the method to raise instead an exception if not found instead of returning null.
We could parametrize this with something like:
```
function getMetaValue(name: string, required: boolean = true)
```
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. This issue will also happen for number (pageNumber, pageSize etc.), so it might be worth.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea to avoid repeating multiple `toString` everywhere. This issue will also happen for number (pageNumber, pageSize etc.), so it might be worth.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just a little concerned that the implementation for casting all values (potentially any type) to string will be hard to keep clean.
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r923260318
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
This could be a good idea, to avoid repeating multiple `toString` everywhere. I am just afraid that the implementation for casting all values (any type) to string will be hard to keep clean.
--
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] bbovenzi commented on pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #25098:
URL: https://github.com/apache/airflow/pull/25098#issuecomment-1192467201
Now that the API type generation is merged do you want to rebase the branch?
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r956298934
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
-export default function useClearRun(dagId, runId) {
+export default function useClearRun(dagId: string, runId: string) {
const queryClient = useQueryClient();
const errorToast = useErrorToast();
const { startRefresh } = useAutoRefresh();
return useMutation(
['dagRunClear', dagId, runId],
- ({ confirmed = false }) => {
+ ({ confirmed = false }: { confirmed: boolean }) => {
const params = new URLSearchParams({
csrf_token: csrfToken,
- confirmed,
+ confirmed: confirmed.toString(),
Review Comment:
Done
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r922656153
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
You are right, most of the `getMetaValue` are required, defaulting to an empty string is not great and this pattern get duplicated everywhere. Raising an exception seems fair.
We could parametrize this with something like:
```
function getMetaValue(name: string, required: boolean = true)
```
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
You are right, most of the `getMetaValue` are required, defaulting to an empty string is not great and this pattern gets duplicated everywhere. Raising an exception seems fair.
We could parametrize this with something like:
```
function getMetaValue(name: string, required: boolean = true)
```
--
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] pierrejeambrun commented on a diff in pull request #25098: Upgrade API file to typescript
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #25098:
URL: https://github.com/apache/airflow/pull/25098#discussion_r922656153
##########
airflow/www/static/js/api/useClearRun.ts:
##########
@@ -17,30 +17,30 @@
* under the License.
*/
-import axios from 'axios';
+import axios, { AxiosResponse } from 'axios';
import { useMutation, useQueryClient } from 'react-query';
import { getMetaValue } from '../utils';
import { useAutoRefresh } from '../context/autorefresh';
import useErrorToast from '../utils/useErrorToast';
-const csrfToken = getMetaValue('csrf_token');
-const clearRunUrl = getMetaValue('dagrun_clear_url');
+const csrfToken = getMetaValue('csrf_token') || '';
+const clearRunUrl = getMetaValue('dagrun_clear_url') || '';
Review Comment:
You are right, most of the `getMetaValue` are required, defaulting to an empty string is not great and this pattern gets duplicated everywhere. Raising an exception seems fair.
We could do something like:
```
function getMetaValue(name: string, required: boolean = true)
```
--
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