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

WIP: #20 Fixes Non-Zero exit code #28

Closed
wants to merge 1 commit into from
Closed

WIP: #20 Fixes Non-Zero exit code #28

wants to merge 1 commit into from

Conversation

jeremyabbott
Copy link
Contributor

Very much WIP
Currently returns non-zero when Fornax build fails.
Fornax build will already return a non-zero if Generator.getPosts fails.
Instead of the solution in this commit, we could throw an exception if the fsi operations fail instead of returning None.

Very much WIP
Currently returns non-zero when `Fornax build` fails.
`Fornax build`  will already return a non-zero if Generator.getPosts fails.
Instead of the solution in this commit, we could throw an exception if the fsi operations fail instead of returning None.
@WalternativE
Copy link
Contributor

Hi @jeremyabbott - I see we're currently working on the same issue (my current WIP is here #27). Do you want to sync on this? :)

@jeremyabbott
Copy link
Contributor Author

@WalternativE that's my mistake! I'm so sorry. I didn't realize someone was already working on it. I'm happy to collaborate. I'm also happy closing this one to work on something else. 😃

@WalternativE
Copy link
Contributor

@jeremyabbott looking at the two PRs I assume that we pretty much came to the same conclusions. At least the code changes indicate that we're trying to get failure/success data out to the calling site to reason about whether the process should be marked as a failure or a success. I'd really like to also hoist other logging side-effects to a higher level (away from the distinct generator handlers) that's why I introduced the typing - I'm still not finished, though. I'd be really happy to hear your thoughts on this. We can then either contribute further or split our efforts on different issues (like the RSS/Atom thing or the fact that there's still no way to update the dll in the _bin directory) ^^

@jeremyabbott
Copy link
Contributor Author

@WalternativE I'm sorry for the delayed response! I wasn't planning on doing a lot more on this PR until we got some clarity on how to resolve the issue. Do we want to just throw on failure, or keep track of the errors. Given that some functions do throw unhandled exceptions, but other ones swallow them, it's unclear what the desired behavior should be.

@WalternativE
Copy link
Contributor

@jeremyabbott no problem, I'm swamped with work myself ^^' I see pros and cons for both approaches. Exceptions aren't so nice regarding 'purity' and the testable interface but we could still use them at the interface where the CLI (calling the generators) and the generator implementations meet. If we let the generators work through the whole thing we could give complete reports about the errors, on the other hand if we fail fast we could just let the user iterate at a faster pace. Thinking about it I'd go with fail fast. I hope I'll have time in the next couple of evenings to implement some of these thoughts.

@Krzysztof-Cieslak
Copy link
Member

I've merged other PR targeting this, so closing this one too

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.

None yet

3 participants