Skip to content

Conversation

@Hartorn
Copy link
Member

@Hartorn Hartorn commented Apr 9, 2025

No description provided.

@Hartorn Hartorn self-assigned this Apr 9, 2025
@Hartorn Hartorn requested a review from henchaves April 9, 2025 08:04
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add timeout limit in both jobs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's useful in this case.
What do you have in mind ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid any job hanging for 6 hours,
we could add timeout-minutes: 30

https://stackoverflow.com/questions/59073731/set-default-timeout-on-github-action-pipeline

run: make doc

test:
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also run in other platforms such as macos and windows, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not matter, we don't use complex lib or math lib

strategy:
fail-fast: false # Do not stop when any job fails
matrix:
python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a TODO to remove 3.9 support after October 2025

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll remove it when we drop the support, added in a linear card

except httpx.HTTPStatusError as e:
if e.response.status_code == httpx.codes.UNPROCESSABLE_ENTITY:
raise ValueError("Validation error: " + e.response.text)
raise ValueError("Validation error: " + e.response.text) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope, should be handled in #44

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's to fix the linting, but you want, I can drop this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean why not using [] as a default ?
This object would be a reference to an list, so the same list would be used for every calls

https://nikos7am.com/posts/mutable-default-arguments/

@henchaves henchaves closed this Apr 9, 2025
@henchaves henchaves reopened this Apr 9, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 9, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@henchaves henchaves changed the title Adding CI workflow chore: add CI workflow Apr 9, 2025
@henchaves henchaves merged commit b38d3a2 into main Apr 9, 2025
15 checks passed
@henchaves henchaves deleted the add-test-run branch April 9, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants