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

feat: Add --file-list <PATH> to choose files to check #708

Merged
merged 1 commit into from
Oct 12, 2023
Merged

feat: Add --file-list <PATH> to choose files to check #708

merged 1 commit into from
Oct 12, 2023

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Apr 6, 2023 •
edited
Loading

Fix #707

Just the draft as I had some time. Please feel free to take over if you'd like, I'm not sure if and when I'll find to finish.

@dpc dpc marked this pull request as ready for review April 7, 2023 02:50
@epage epage mentioned this pull request Apr 8, 2023
Closed
@dpc dpc mentioned this pull request Apr 9, 2023
Merged
@dpc dpc force-pushed the stdin-paths branch 2 times, most recently from fdde0b3 to 4e82c55 Compare April 15, 2023 05:27
@dpc dpc requested a review from epage April 15, 2023 05:29
epage
epage reviewed Apr 17, 2023
View reviewed changes
crates/typos-cli/src/bin/typos-cli/main.rs Outdated Show resolved Hide resolved
crates/typos-cli/src/bin/typos-cli/main.rs Outdated Show resolved Hide resolved
crates/typos-cli/src/bin/typos-cli/main.rs Outdated Show resolved Hide resolved
@epage epage mentioned this pull request Aug 21, 2023
Closed
epage added a commit to epage/typos that referenced this pull request Aug 21, 2023
@epage
fix(action): Don't pre-filter checked files
286609c 
This reverts commit fc7f517.

This has two problems
- This doesn't correctly handle spaces, likely needing crate-ci#708
- This overrides excludes, see crate-ci#347

This also has a weird cost/benefit balance because this requires enough
repo history to do the analysis which takes time to pull down.

Rather than waiting until the relevant changes are in to make this work,
I'm pulling this out for now.

Fixes crate-ci#806
@epage epage mentioned this pull request Aug 21, 2023
Merged
@dpc
Copy link
Contributor Author

dpc commented Sep 19, 2023 •
edited
Loading

Ehh. I don't know how to properly handle this on Windows. I don't know the platform, and it seems it might actually require checking the current system encoding and some other stuff like that?

It's silly. Actual users could have been enjoying this feature for months now, and we stuck trying to support all these 0.7 users of typos on Windows, none of which even know what 0-separated strings on stdin are. Can Windows even do it? Not supporting it on Windows does not break any existing feature for Windows users. I've been using Linux for more than 2 decades, and I don't remember Windows developers being very concerned about compatibility with other OSes in the past.

Here is my solution for Windows users who need this feature: either submit a PR, or here is $0 needed to install a better operating system: 💵 .

@dpc dpc force-pushed the stdin-paths branch 2 times, most recently from c66dc56 to 4ca840b Compare October 11, 2023 07:18
@dpc
Copy link
Contributor Author

dpc commented Oct 11, 2023 •
edited
Loading

I have rebased on top of #837 which fixes one pain point of this PR I was hitting, so extend-exclude works now for --stdin-paths.

Would be nice if we could land. It's a really useful feature. Have been using it from my fork all this time. If there's anything other than obsolete OSes support, I'd be happy to address it. I would really fix it even for Windows if I could. I really have better thing to do than annoy you about it. 🤣

@dpc dpc changed the title chore: add --stdin-paths and -stdin-paths0 chore: add --file-list Oct 11, 2023
@dpc dpc force-pushed the stdin-paths branch 2 times, most recently from e2b9743 to 358f44f Compare October 11, 2023 21:06
@epage epage changed the title chore: add --file-list feat: Add --file-list <PATH> to choose files to check Oct 11, 2023
@epage
Copy link
Collaborator

epage commented Oct 11, 2023

I believe the comments I just made are the extent of what is needed and I can merge after this.

(nit: this would be a "feat" type, rather than a "chore" though this PR process might have felt like one...)

@dpc
Copy link
Contributor Author

dpc commented Oct 11, 2023

(nit: this would be a "feat" type, rather than a "chore" though this PR process might have felt like one...)

Features are chores too.

@dpc
Copy link
Contributor Author

dpc commented Oct 11, 2023

@epage done

@epage
Copy link
Collaborator

epage commented Oct 12, 2023

Features are chores too.

I'm curious, how do you define the "chore" type? I'm not seeing any more formal definition. I tend to use it for non-user-impacting project bookkeeping (e.g. changing CI pipelines). Or from another perspective, I view it as "this doesn't go in a changelog" while a feat and a fix would.

@epage epage enabled auto-merge October 12, 2023 02:31
@epage epage merged commit 4448cda into crate-ci:master Oct 12, 2023
14 checks passed
@dpc
Copy link
Contributor Author

dpc commented Oct 12, 2023

I'm curious, how do you define the "chore" type? I'm not seeing any more formal definition. I tend to use it for non-user-impacting project bookkeeping (e.g. changing CI pipelines). Or from another perspective, I view it as "this doesn't go in a changelog" while a feat and a fix would.

I suck at conventional commits and I'm not even sure why I'm trying...

I tend to use it ...

Makese sense. I should try to remember it. "Doesn't go in a changelog"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: pass file list through stdin
2 participants
@dpc @epage