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

Process file list #115

Merged
merged 19 commits into from
Jan 10, 2020
Merged

Process file list #115

merged 19 commits into from
Jan 10, 2020

Conversation

norman-abramovitz
Copy link
Contributor

@norman-abramovitz norman-abramovitz commented Dec 31, 2019

Added code to process multiple files

resolves #54

@norman-abramovitz norman-abramovitz changed the title Process multiple filename Process file list Dec 31, 2019
Copy link
Owner

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

Thanks.

Please address my remarks, rebase on latest master and squash all comments into a single one.

Also, just being curious, but what happens if you give - twice, as in mdcat - -? Does that fail? Or freeze?

src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@swsnr swsnr added the enhancement New feature or request label Jan 3, 2020
@norman-abramovitz
Copy link
Contributor Author

"mdcat - -" does not work through the cli parser. It detects one of the dashes as an incomplete argument. It would also fail in pipelines when the first stdin would close and the pipeline is closed.

I will make the changes over the weekend when I get some me time. 😄

@norman-abramovitz
Copy link
Contributor Author

Correction. I was running the older mdcat command. The "mdcat - -" does work but it will still make pipelines fail.

@swsnr
Copy link
Owner

swsnr commented Jan 3, 2020

I’m sorry, can you please fix CI?

@norman-abramovitz
Copy link
Contributor Author

I am going to delete this PR since the errors are coming from code I did not modify and start over. The code I have compiles just fine.

@swsnr
Copy link
Owner

swsnr commented Jan 3, 2020

I didn’t check the actual error on CI, perhaps I made a mistake in the CI setup 🤷‍♂️. If you like I can check 🙂

@norman-abramovitz
Copy link
Contributor Author

I did not touch formatting.rs or lib.rs for the latest fixes. I going to double-check the merge as well.

@norman-abramovitz
Copy link
Contributor Author

Figured it out. It was in the tests cases.

@norman-abramovitz
Copy link
Contributor Author

I am not seeing a way squash commits after the fact.

@swsnr
Copy link
Owner

swsnr commented Jan 4, 2020

Use “git rebase -i” and force-push the result. There are lots of tutorials about this on the internet 🙂

Copy link
Owner

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

Oh, I forgot another thing: Would you mind adding a CHANGELOG.md entry for this pull request? :)

src/main.rs Outdated Show resolved Hide resolved
@norman-abramovitz
Copy link
Contributor Author

Learned something new about git. I knew about the rebase command, but I did not know I could do it on a remote forked repository. All the examples were on the master and in PR case, we need to use upstream/master. It got me into conflict detect h-ll but figured it out after a @while.

Norman Abramovitz added 8 commits January 4, 2020 09:00
2) Redid ownership on TerminalCapabilities
3) Undo debugging code on the arg processing loop
4) Corrected exit return value
5) Make capabilities a reference for the test cases
6) Add change log entry
2) Redid ownership on TerminalCapabilities
3) Undo debugging code on the arg processing loop
4) Corrected exit return value
5) Make capabilities a reference for the test cases
6) Add change log entry
2) Redid ownership on TerminalCapabilities
3) Undo debugging code on the arg processing loop
4) Corrected exit return value
5) Make capabilities a reference for the test cases
6) Add change log entry
2) Redid ownership on TerminalCapabilities
3) Undo debugging code on the arg processing loop
4) Corrected exit return value
5) Make capabilities a reference for the test cases
6) Add change log entry
CHANGELOG.md Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@swsnr
Copy link
Owner

swsnr commented Jan 6, 2020

I'm sorry but this:

commits

doesn't look quite right, does it? Why the same message so many times, and what are all these merge commits? Also, why do you prefix the commit message with 1)?

@norman-abramovitz
Copy link
Contributor Author

norman-abramovitz commented Jan 6, 2020 via email

--fail-fast if you want to exit on the first failure.
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
Norman Abramovitz added 8 commits January 10, 2020 02:30
Redid ownership on TerminalCapabilities.
Make capabilities a reference for the test cases.
Add change log entry
Converted code from fail-fast to fail-safe
Refactor process_arguments into main
Added --fail flag to exit on the first file error

refactor process_arguments into main
Copy link
Owner

@swsnr swsnr left a comment

Choose a reason for hiding this comment

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

Looks really great now 😍

I've got a minor nitpick about the changelog; if you could just address that I'll merge the pull request and make a new release this weekend :)

Thank you very very much for this feature, and for bearing with all my nitpicking 🙏

CHANGELOG.md Outdated Show resolved Hide resolved
@swsnr swsnr merged commit 4e70ee7 into swsnr:master Jan 10, 2020
swsnr added a commit that referenced this pull request Jan 10, 2020
@norman-abramovitz norman-abramovitz deleted the add_args branch January 10, 2020 17:01
@swsnr
Copy link
Owner

swsnr commented Jan 11, 2020

@norman-abramovitz Released 0.15.0 with your change. Thanks again :)

@norman-abramovitz
Copy link
Contributor Author

@lunaryorn You're welcome. BTW you might want to run cargo update sometime in the near future. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow file list as input
2 participants