Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make compose.py's exit code 1 if an error was logged #38084

Merged
merged 1 commit into from
May 8, 2020
Merged

Make compose.py's exit code 1 if an error was logged #38084

merged 1 commit into from
May 8, 2020

Conversation

ifreund
Copy link
Contributor

@ifreund ifreund commented Feb 17, 2020 •
edited
Loading

Summary

SUMMARY: None

Purpose of change

This will make Ultica's CI fail if an error is logged while composing which will be helpful in preventing PRs with errors from being merged in the future.

Describe the solution

Create a global to track error state and exit with code 1 if an error occured. This is a bit of a hack, but usage of a global seems to be the simplest an least invasive way to do this

Testing

Tested that composing Ultica (which currently has some errors) exited with code 1.

Additional context

This shouldn't be merged until @I-am-Erk has signed off on it and is ready for CI builds of Ultica to fail on errors.

@ifreund ifreund added the [Python] Code made in Python label Feb 17, 2020
@ZhilkinSerg ZhilkinSerg added Organization General development organization issues SDL: Tiles / Sound Tiles visual interface and sounds. labels Feb 26, 2020
@weekly-digest weekly-digest bot mentioned this pull request Mar 1, 2020
Closed
@stale
Copy link

stale bot commented Mar 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Mar 18, 2020
@Night-Pryanik Night-Pryanik removed the stale Closed for lack of activity, but still valid. label Mar 18, 2020
@ZhilkinSerg
Copy link
Contributor

@I-am-Erk are you ready to fail CI tests for Ultica?

@stale
Copy link

stale bot commented May 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label May 2, 2020
@ZhilkinSerg ZhilkinSerg added (P5 - Long-term) Long-term WIP, may stay on the list for a while. and removed stale Closed for lack of activity, but still valid. labels May 2, 2020
This was referenced May 3, 2020
Closed
Closed
@kevingranade
Copy link
Member

According to https://github.com/I-am-Erk/CDDA-Tilesets/runs/638846035 it looks like there are a number of failures, i.e. sprite id 't_dirt_spring' has no matching PNG file. It will not be added to tile_config.json
IDK what is involved in addressing these so I'm hesitant to pull the trigger.

@ifreund
Copy link
Contributor Author

ifreund commented May 3, 2020

Pinging @barsoosayque since Erk seems to be pretty busy with life these days.

@barsoosayque barsoosayque mentioned this pull request May 5, 2020
Merged
@ifreund ifreund force-pushed the compose-exit-error-code branch from e350ac0 to 227a5c7 Compare May 5, 2020 21:07
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2020

This pull request introduces 2 alerts when merging 227a5c7 into 6f77ec7 - view on LGTM.com

new alerts:

  • 1 for Unreachable code
  • 1 for Use of 'global' at module level

@barsoosayque
Copy link
Contributor

Ultica is fixed and 100% ready to fail !

@ifreund
Copy link
Contributor Author

ifreund commented May 6, 2020

Wonderful! Unfortunately it looks like what I've done is considered bad python and LGTM is now here to tell us about it. Will fix.

@ifreund
Make compose.py's exit code 1 if error logged
f180b55 
This is a bit of a hack, but usage of a global seems to be the minimally
invasive way to do this. Also categorized the messages into Info,
Warning, and Error types.
@ifreund ifreund force-pushed the compose-exit-error-code branch from 227a5c7 to f180b55 Compare May 7, 2020 09:21
@ifreund
Copy link
Contributor Author

ifreund commented May 7, 2020

Alright I cleaned things up a bit. Hopefully lgtm likes it.

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2020

This pull request introduces 1 alert when merging f180b55 into b8fb092 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@ifreund
Copy link
Contributor Author

ifreund commented May 7, 2020

Well, LGTM is wrong. That code is very much reachable

@kevingranade
Copy link
Member

kevingranade commented May 7, 2020 via email

@kevingranade kevingranade merged commit 77b1f5f into CleverRaven:master May 8, 2020
@ifreund ifreund deleted the compose-exit-error-code branch May 8, 2020 08:08
This was referenced May 10, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barsoosayque barsoosayque barsoosayque left review comments

@mlangsdorf mlangsdorf mlangsdorf left review comments

Assignees
No one assigned
Labels
Organization General development organization issues (P5 - Long-term) Long-term WIP, may stay on the list for a while. [Python] Code made in Python SDL: Tiles / Sound Tiles visual interface and sounds.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ifreund @ZhilkinSerg @kevingranade @barsoosayque @mlangsdorf @Night-Pryanik