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

feat: continue-on-error flag for handling errors while processing multiple files #565

Merged
merged 8 commits into from
Apr 2, 2025

Conversation

iyilmaz24
Copy link
Contributor

  • Currently: If multiple files are specified, mark starts processing them but exits on the first error - leaving some files untouched
  • Proposed: Adding a continue-on-error flag, that does not change above behavior unless set to true, which in that case will process the entire batch of files & log any encountered errors to console.
  • Purpose: This should improve user's experience using mark with batch files, as it makes subsequent runs for error'd batches easier

I would like the maintainer's thoughts on the overall idea / PR but also:

  • Formerly on some errors, we only logged the error
  • Screenshot 2025-02-19 at 3 26 07 PM
  • Now in addition, we can also log a helpful string message. I commented out all the changed errors throughout the code and would like to hear thoughts on the new string messages and which ones you dislike or want to replace.

I have also added some tests in a temporary batch-tests directory so that the proposed functionality can be tested.

  • This command can be used for testing ./mark --compile-only --continue-on-error -files "**/testdata/batch-tests/*.md"

Note: there is a pattern where we handle the error with fatalErrorHandler and then return nil after. I was not able to directly return fatalErrorHandler(args...) due to the processFile function signature returning *confluence.PageInfo. Any sidesteps / alternatives to this are also welcomed (we are able to return nil since a pointer can be nil).

Possible further enhancements: Outputting a errors.txt file for users containing the names (and maybe also error messages) of all the error'd files for easy retrial

@iyilmaz24 iyilmaz24 marked this pull request as ready for review February 24, 2025 16:14
@iyilmaz24
Copy link
Contributor Author

Hi, I was wondering if this was ready to merge - I was looking into creating some batch tests for this functionality. Would it be better to add them to this PR or make a new PR?

@mrueg
Copy link
Collaborator

mrueg commented Mar 6, 2025

feel free to include them here, it would be good if they were executable using the regular tests or bats acceptance tests if necessary. Currently the PR needs to be rebased, as there are file conflicts.

@iyilmaz24
Copy link
Contributor Author

Hi, the batch tests have been added - with tons of help from @richscott (thanks!)

We also made the decision to refactor/relocate some of the code from the main.go file into a util package. It was necessary to import certain functions and structs for the new batch tests which wasn't possible with the previous code organization. We hope this code relocation also helps in the future with extensibility. Excited to hear any thoughts on the changes 🙂

@mrueg
Copy link
Collaborator

mrueg commented Mar 12, 2025

Thanks for adding tests for this! Can you rebase on the latest master? Otherwise I won't be able to merge the changes.

Making main.go slim was something I had in mind as well as part of #499 so I don't mind the temporary utils package (utils is usually not the best pattern, because it just means that random stuff gets dropped into a single package), I'll work on the reorganization later once urfave/cli/v3 is out (splitting CLI parsing from the rest of the logic).

iyilmaz24 and others added 8 commits March 12, 2025 14:54
- add continue-on-error flag as a command line option
    - if set, doesnt exit on error and continues
        processing other files that were passed in
- add fatalErrorHandler to handle fatal errors
    - if continue-on-error flag is set, does not exit
- add temporary tests for continue-on-error flag
    - add tests in batch-tests subdirectory
- Fix incorrect argument passing to log.Errorf and log.Fatalf functions
- Remove debugging comments, temporary tests, and print statements
- Address linter warnings related to error logging
Move a number of funcs/files in the top-level `main` package into a new
`util` package, so test logic can directly invoke functions like
RunMark(), etc.  The main.go has been trimmed down to minimal sizing,
with former supporting funcs moved into `util` package, so they
can be run by unit tests.

Signed-off-by: Rich Scott <[email protected]>
@iyilmaz24 iyilmaz24 force-pushed the batch-process-COE-flag branch from bf3903c to 4778a8c Compare March 12, 2025 19:06
@iyilmaz24
Copy link
Contributor Author

Hello, I was able to rebase the PR onto the master branch and it should be good to go. 🙂

@richscott
Copy link
Contributor

I agree completely about the util/ directory - it's definitely an example of the "utils folder kitchen-junk-drawer" anti-pattern. I thought about different package topologies, but didn't feel confident about any that would be durably correct, so out of expediency I made util for now, so Irfan could get the continue-on-error logic into a unit-test.

@richscott
Copy link
Contributor

HI, @mrueg - can you (or anyone else with privileges on this repo) restart these CI checks on this PR? They had the error The job was not started because recent account payments have failed or your spending limit needs to be increased. Please check the 'Billing & plans' section in your settings - I assume it's a transient error, maybe? We'd love to see this PR merged - thanks.

@mrueg
Copy link
Collaborator

mrueg commented Mar 19, 2025

HI, @mrueg - can you (or anyone else with privileges on this repo) restart these CI checks on this PR? They had the error The job was not started because recent account payments have failed or your spending limit needs to be increased. Please check the 'Billing & plans' section in your settings - I assume it's a transient error, maybe? We'd love to see this PR merged - thanks.

I've reached out to @kovetskiy a week ago, hope he find some time to respond.

@mrueg mrueg merged commit a0c6abf into kovetskiy:master Apr 2, 2025
5 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants