Skip to content

Allow specifying glob pattern to ignore config option #3488

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
Merged
merged 4 commits into from
Apr 14, 2019
Merged

Allow specifying glob pattern to ignore config option #3488

merged 4 commits into from
Apr 14, 2019

Conversation

rchaser53
Copy link
Contributor

related: #3413

@rchaser53 rchaser53 changed the title [WIP] Allow specifying glob pattern to ignore config option Allow specifying glob pattern to ignore config option Apr 3, 2019
scampi
scampi reviewed Apr 8, 2019
View reviewed changes
topecongiro
topecongiro suggested changes Apr 9, 2019
View reviewed changes
src/ignore_path.rs Outdated
FileName::Real(p) => !self
.ignore_set
.matched_path_or_any_parents(p, false)
.is_none(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use is_ignore() instead of ! and is_none().

src/config/mod.rs Outdated
@@ -135,7 +135,7 @@ create_config! {
"Report all, none or unnumbered occurrences of TODO in source file comments";
report_fixme: ReportTactic, ReportTactic::Never, false,
"Report all, none or unnumbered occurrences of FIXME in source file comments";
ignore: IgnoreList, IgnoreList::default(), false,
ignore: IgnoreList, IgnoreList::default(), true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please not make ignore stable in this PR? I would prefer not to mark it stable until it gets tested a bit more.

@topecongiro topecongiro mentioned this pull request Apr 9, 2019
Closed
@rchaser53
repalce globset to ignore
c270a2c 
- fix from_ignore_list
- remove dir from ignore_list
- add test
- ! and is_none => is_ignore
@rchaser53
Copy link
Contributor Author

Thank you the review. I changed my code.
Should I not use rebase for review easily?

topecongiro
topecongiro reviewed Apr 11, 2019
View reviewed changes
Cargo.toml Outdated

# A noop dependency that changes in the Rust repository, it's a bit of a hack.
# See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust`
# for more information.
rustc-workspace-hack = "1.0.0"
ignore = "0.4.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please put this under dirs = "1.0.4"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh Sorry.

@topecongiro
Copy link
Contributor

Should I not use rebase for review easily?

You don't have to rebase, I'll just squash and merge when the PR is ready.

@rchaser53
Copy link
Contributor Author

Should I not use rebase for review easily?

You don't have to rebase, I'll just squash and merge when the PR is ready.

I see. I will not rebase since next time.

@topecongiro topecongiro merged commit 34bf137 into rust-lang:master Apr 14, 2019
@rchaser53 rchaser53 deleted the ignore-with-glob branch April 14, 2019 10:31
@euclio
Copy link
Contributor

euclio commented Apr 22, 2019

I think this may have broken how ignore handles workspaces? I have an ignore file at the workspace root that specifies that certain files must not be formatted within dependent crates. It works as expected with rustfmt 1.2.0-nightly (09940a70 2019-03-27) on my local machine but seems to have broken with a more recent rustfmt: https://travis-ci.org/Hoverbear/rust-rosetta/builds/523126071

I'm currently unable to rustup to the same version of rustfmt that my CI is using so I'm unable to test it locally and see if it's workspace-related.

@topecongiro topecongiro mentioned this pull request Apr 22, 2019
Closed
@topecongiro
Copy link
Contributor

@euclio Thank you for reporting the issue. I could confirm it locally and created an #3521 to track it.

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

@scampi scampi scampi left review comments

@topecongiro topecongiro topecongiro requested changes

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

Successfully merging this pull request may close these issues.
4 participants
@rchaser53 @topecongiro @euclio @scampi