Skip to content

Conversation

davidangarita1
Copy link

automatic update of plotly.js

@ayjayt ayjayt self-requested a review August 18, 2025 18:11
Copy link
Collaborator

@ayjayt ayjayt left a comment

Choose a reason for hiding this comment

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

non-zero or zero exit, it matters:

if sys.exit(0), the action is marked as success.

if sys.exit(1), the action is marked as a failure.

When should it marked as failure? When as success?

Should be marked as failure when the user needs to be notified as something.

Careful with the prints. It would be nice if w/ action success/failure status, the subtitle was whatever link the user had to follow. This is what github pages does when a new pages is published.

Can this also automatically update the CHANGELOG is a second commit?

sys.exit(0)

if pr.decode():
print(f"Pull request '{branch}' already exists")
Copy link
Collaborator

Choose a reason for hiding this comment

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

exit non-zero and print to stderr

Copy link
Author

@davidangarita1 davidangarita1 Aug 19, 2025

Choose a reason for hiding this comment

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

In this case, stderr prints empty if there's a PR with that branch

PS: Translated with AI

Copy link
Collaborator

@ayjayt ayjayt Oct 17, 2025

Choose a reason for hiding this comment

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

pero por que? explicame toda la logica porfa

Copy link
Author

Choose a reason for hiding this comment

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

Vale, en este endpoint decidi usar una bandera de gh llamada --silence que solo devuelve algo cuando no encuentra la rama, como solo es una validacion no veo la necesidad de procesar el json que devuelve si no le mando silence.

@gvwilson gvwilson added feature something new P1 needs immediate attention labels Aug 22, 2025
@ayjayt ayjayt marked this pull request as draft August 30, 2025 02:08
Copy link
Collaborator

@ayjayt ayjayt left a comment

Choose a reason for hiding this comment

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

mayoria de acciones deben estar un action.yml en su propio repositorio para reusar

Edit: im ok for now with the current strategy but think changelogtxt-parsers should have an action.

@ayjayt
Copy link
Collaborator

ayjayt commented Oct 8, 2025

@davidangarita1 some changes were not resolved and I want to re-review the script. Please add more logging, especially intermediate values.

Copy link
Collaborator

@ayjayt ayjayt left a comment

Choose a reason for hiding this comment

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

ok i just dont understand why you are not printing more to stderr.

stdout would be like if the user is expecting some out. if something unexpected happens, we print it to stderr. i think we can print to stderr and still return 0 from program if its unexpected but ok.

but not a big deal, i will approve.

@ayjayt
Copy link
Collaborator

ayjayt commented Oct 17, 2025

@davidangarita1 please merge this with main and rerun uv sync to make the uv.lock file updated.

@ayjayt ayjayt marked this pull request as ready for review October 17, 2025 18:30
@ayjayt
Copy link
Collaborator

ayjayt commented Oct 17, 2025

@davidangarita1 once it is accepted remind me and we will trigger it automatically.

tag-check: ${{ env.IS_TAG_PUSH == 'true' && env.TAG || '' }}
summarize-news: ${{ env.IS_PR_TO_DEFAULT == 'true' && 'origin.txt updated.txt' || '' }}
- name: Check .editorconfig exists
run: |
Copy link
Collaborator

@ayjayt ayjayt Oct 17, 2025

Choose a reason for hiding this comment

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

i'm gonna change some things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature something new P1 needs immediate attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants