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.

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

Allow treating warnings as errors #26

Closed
jklausa opened this issue Oct 5, 2021 · 8 comments · Fixed by #28
Closed

Allow treating warnings as errors #26

jklausa opened this issue Oct 5, 2021 · 8 comments · Fixed by #28

Comments

@jklausa
Copy link

jklausa commented Oct 5, 2021

Heyo :)

I love that you wrote this (and open-sourced!), since it solved a big pain point I've had before with localisation tools not being harsh enough in validation.

Would you consider adding a semi-stable (it's before 1.0, so... whatever goes) output format that could be parsed by other tools? My use-case is to have locheck be part of our CI suite, so we can make sure the strings we're merging are correct, but it's currently not super nice for that use-case. (I'm currently parsing the stdout output, and looking for lines that begin with "WARNING: or ERROR:, which, workable, but... yeah.)

JSON would probably be the easiest?

And I know that the README says:

Pull requests for additional output formats will probably be accepted.

but I wanted to check in with y'all if this is something you'd be even interested in / maybe are working on already, before I start hacking on it.

@irskep
Copy link

irskep commented Oct 5, 2021

We are interested, but are waiting for someone to ask for a specific format. "JSON" is too broad because the schema can be anything. We only use the xcodebuild output format internally. Does your CI suite have an existing XML or JSON schema it uses to report errors?

@jklausa
Copy link
Author

jklausa commented Oct 5, 2021

We only use the xcodebuild output format internally.

I'm guessing/assuming you mean you're using this as a "Build Phase" script?
That means incurring a time penalty for each build, even when you didn't touch strings, right?

Does your CI suite have an existing XML or JSON schema it uses to report errors?

Not really, we just examine the CI logs to see what's broken.

I don't really need anything fancy here, I just want something I can parse in Ruby and then either fail the CI run by exit(1)ing, or report a success.

@irskep
Copy link

irskep commented Oct 5, 2021

Yes, we use it as a build phase, but only in CI, not locally. We also just read the logs to see what broke—it's why I added the 'summary' feature on stdout. If it fails we can quickly see which strings need to be fixed without trying to read the xcodebuild output.

You don't need to parse the output to check for success or failure, just look for a nonzero exit code. If the exit code is zero, there were no errors, only warnings. If there were errors, the stdout summary should have everything you need.

If you would prefer that warnings also trigger a nonzero exit code, we should add a flag for that, not a new output format. :-)

@jklausa
Copy link
Author

jklausa commented Oct 6, 2021

If you would prefer that warnings also trigger a nonzero exit code, we should add a flag for that, not a new output format. :-)

You know what? I'm 100% sure I had that thought at one point when working on this, but then I was like "huh, I wonder if I can hack it somehow", ended up doing some weird parsing stuff, and then my mind went to "sure would be nice to be able to parse that more easily!"; instead of circling back to "I want warnings as errors".

Should I open another issue for --treat-warnings-as-errors or whatever ;P?

@irskep
Copy link

irskep commented Oct 6, 2021

Yes, that sounds good. It should be extremely easy to implement and you're welcome to take a crack at it, since I won't be working on this for at least another 3-4 weeks.

@jklausa
Copy link
Author

jklausa commented Oct 25, 2021

Things got reshuffled for me at $dayJob and I didn't have time to work on this yet, but I'll take a stab at it this week :)

@stevelandeyasana stevelandeyasana changed the title Provide a stable-ish, machine-readable format Allow treating warnings as errors Nov 2, 2021
@stevelandeyasana
Copy link
Collaborator

Done! #28.

@stevelandeyasana
Copy link
Collaborator

This change has been released as part of 0.9.4.

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

Successfully merging a pull request may close this issue.

3 participants