Skip to content

#476 add missing complete-foreign-keys option to bin #477

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

Merged
merged 1 commit into from
Jun 9, 2017
Merged

#476 add missing complete-foreign-keys option to bin #477

merged 1 commit into from
Jun 9, 2017

Conversation

thedanielhanke
Copy link
Contributor

@thedanielhanke thedanielhanke commented Jun 8, 2017

adds the show_complete_foreign_keys flag to the cli, which i forgot. see #475.

i named it --ck in short, to have it aligned under -k hence is why i also set the show_foreign_keys option positive.

had to increase the Metrics/BlockLength a bit to make rubocop pass. Since bin/annotate is a good candidate for even more lines, i`d recommend to keep smaller blocks and exclude the file from this check.
However, this is a entirely different discussion which is why i did the increment only.

Did not find a spec for the bin options itself. is it missing or is it just me?

also, sorry all for the wrong branch name after an creating a useless ticket, bad memory.

@thedanielhanke
Copy link
Contributor Author

@steveklebanoff ^

@steveklebanoff
Copy link
Contributor

image

Formatting looks a little funky on github, description should be on same line. Besides that, looks good to me as long as @ctran is cool with the rubocop change

@thedanielhanke
Copy link
Contributor Author

@steveklebanoff the formatting is due to the length of the key.
i think complete-foreign-keysComplete... will not look more decent :P

i could however, indent the entire right readme section a bit, or remove one of the two options.
The optionparser would need to receive the same change:

$ ./bin/annotate --help | ag -B1 -A2 '\--ck' 
    -k, --show-foreign-keys          List the table's foreign key constraints in the annotation
        --ck, --complete-foreign-keys
                                     Complete foreign key names in the annotation
    -i, --show-indexes               List t....

which is all unrelated somehow. so i'd await @ctran`s opinion on this.
suggestions from you, @steveklebanoff ?

@thedanielhanke
Copy link
Contributor Author

@steveklebanoff I forgot the most important part: thanks for your review! 👍

@ctran
Copy link
Owner

ctran commented Jun 9, 2017

Thanks much!

@ctran ctran merged commit 134b833 into ctran:develop Jun 9, 2017
@ctran ctran added this to the v2.7.3 milestone Jun 9, 2017
@thedanielhanke thedanielhanke deleted the 476_add_complete_foreign_keys_option_to_bin branch June 9, 2017 11:02
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