Skip to content

[CLI] implement regex filtering#1914

Closed
KeenS wants to merge 23 commits intodiesel-rs:masterfrom
KeenS:filter-regex
Closed

[CLI] implement regex filtering#1914
KeenS wants to merge 23 commits intodiesel-rs:masterfrom
KeenS:filter-regex

Conversation

@KeenS
Copy link
Contributor

@KeenS KeenS commented Nov 11, 2018

Implement #1911
To hold backward compatibility, added new fields *_table_regexes.

@weiznich weiznich requested a review from a team November 11, 2018 15:23
@KeenS
Copy link
Contributor Author

KeenS commented Nov 15, 2018

blocked by #1889 , i guess.

@weiznich
Copy link
Member

weiznich commented Nov 20, 2018

blocked by #1889 , i guess.

Yes I would like to merge #1889 before merging any bigger PR. (Also maybe releasing 1.4)

@KeenS KeenS changed the title [CLI] implement regex filtering [WIP] [CLI] implement regex filtering Jan 29, 2019
@KeenS
Copy link
Contributor Author

KeenS commented Jan 31, 2019

Some jobs of CI on Azure is failing with unknown reason and some are canceled somehow.

@KeenS KeenS changed the title [WIP] [CLI] implement regex filtering [CLI] implement regex filtering Jan 31, 2019
@KeenS
Copy link
Contributor Author

KeenS commented Jan 31, 2019

force rerun resolved everything.

@KeenS
Copy link
Contributor Author

KeenS commented Mar 11, 2019

@weiznich It's ready to review.

weiznich
weiznich reviewed Mar 11, 2019
View reviewed changes
diesel_cli/src/cli.rs Outdated
.short("o")
.long("only-tables")
.help("Only include tables from table-name")
.conflicts_with("only-table-regexes")
Copy link
Member

@weiznich weiznich Mar 11, 2019

Choose a reason for hiding this comment

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

The next release will be 2.0 so we can make a breaking change here. Therefore I would prefer to just introduce this functionality under the old flags.

cc @diesel-rs/core as information.

Copy link
Contributor Author

@KeenS KeenS Sep 30, 2019

Choose a reason for hiding this comment

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

What does the old flags mean?

Copy link
Member

@weiznich weiznich Sep 30, 2019

Choose a reason for hiding this comment

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

I'm thinking of just replacing the old flag (only-tables) with the regex filtering.

diesel_cli/src/main.rs Outdated
let filter_regex = matches
.values_of("table-name-regexpes")
.unwrap_or_default()
.map(|table_name_regex| Regex::new(table_name_regex).unwrap().into())
Copy link
Member

@weiznich weiznich Mar 11, 2019

Choose a reason for hiding this comment

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

Could we print a nicer error message here if the provided string is no valid regex?

(Hint: If that map returns a Result it is possible to collect into a Result<Vec<_>, _>)

@KeenS
Copy link
Contributor Author

KeenS commented Mar 12, 2019

fixed message. And also, I found the filtering is referencing non-existent argument. Thus fixed to refer "table-name"

$ cargo run -p diesel_cli --features postgres --no-default-features -- print-schema --except-table-regexes  'a('
   Compiling diesel_cli v1.4.0 (/home/shun/Rust/diesel/diesel_cli)
    Finished dev [unoptimized + debuginfo] target(s) in 4.28s
     Running `/home/shun/Rust/diesel/target/debug/diesel print-schema --except-table-regexes 'a('`
invalid argument for table filtering regex: regex parse error:
    a(
     ^
error: unclosed group

@KeenS
Copy link
Contributor Author

KeenS commented Mar 12, 2019

Nightly tests are failing because try_from has been stabilized. It's not relevant to this PR

@KeenS
Copy link
Contributor Author

KeenS commented Jul 25, 2019

wait on #2108

@JohnTitor JohnTitor requested a review from a team August 6, 2019 20:32
@AlterionX
Copy link

AlterionX commented Sep 28, 2019

@weiznich @KeenS So is this one good to go and is just waiting for the next major version release? If so, would it be prudent to add in a label or a milestone/project for that? Or is functionality going to be moved back under the old flags?

@KeenS
Copy link
Contributor Author

KeenS commented Oct 4, 2019

error: 'rustc' is not installed for the toolchain 'beta-x86_64-unknown-linux-gnu'
To install, run rustup component add rustc --toolchain beta-x86_64-unknown-linux-gnu

Maybe travis isn't up-to-date?

@TaKO8Ki TaKO8Ki mentioned this pull request Sep 24, 2020
Merged
@KeenS
Copy link
Contributor Author

KeenS commented Sep 28, 2020

closing in favor of #2515. Thank you @TaKO8Ki for taking over my work.

@KeenS KeenS closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weiznich weiznich weiznich left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KeenS @weiznich @AlterionX