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

Format should not crash #344

Merged
merged 7 commits into from
Jul 18, 2018
Merged

Format should not crash #344

merged 7 commits into from
Jul 18, 2018

Conversation

rambleraptor
Copy link
Contributor

@rambleraptor rambleraptor commented Jul 11, 2018

The format() function should never crash.

If it has to, it should return the best possible choice (almost certainly the last) with Rubocop disable/enable clauses surrounding it.

This should be a no-op whatever the polar opposite of a no-op is.


[all]

Format should not crash

[terraform]

[puppet]

[puppet-compute]

[puppet-container]

[puppet-dns]

[puppet-logging]

[puppet-pubsub]

[puppet-resourcemanager]

[puppet-sql]

[puppet-storage]

[chef]

[chef-compute]

[chef-container]

[chef-dns]

[chef-logging]

[chef-sql]

[chef-storage]

[ansible]

Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

Great! 👍

Let's also increase size from 80 to 100, what do you think about that?

@rambleraptor
Copy link
Contributor Author

Yeah, I can do that. @danawillow thoughts?

@modular-magician
Copy link
Collaborator

I am a robot that works on MagicModules PRs!
I checked the downstream repositories (see README.md for which ones I can write to), and none of them seem to have any changes.

Once this PR is approved, you can feel free to merge it without taking any further steps.

@modular-magician
Copy link
Collaborator

I am a robot that works on MagicModules PRs!
I checked the downstream repositories (see README.md for which ones I can write to), and none of them seem to have any changes.

Once this PR is approved, you can feel free to merge it without taking any further steps.

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Sure!

@rambleraptor
Copy link
Contributor Author

I bumped up the line length from 80 -> 100.

The submodule PRs for these ones are gonna be a lot of fun.

@rambleraptor rambleraptor force-pushed the better-format branch 2 times, most recently from bec9409 to 870224f Compare July 13, 2018 22:02
@modular-magician
Copy link
Collaborator

it 'fits 80 chars' do
subject.format [['x' * 80]]
it 'fits 100 chars' do
subject.format [['x' * 100]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we don't raise an exception, you should expect this not to include a line length disable. Same with all other tests that used to depend on crashes to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests should verify that format works properly, but I don't think that includes the "LineLength" portion.

Check out the changes. I think the better way to verify that if < 80, the output is exactly as expected, where > 80 includes LineLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, pushes on Github are being weird so I'll probably have to re-push + fiddle with the branch in a bit.

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 83ddcf2) have been included in your existing downstream PRs.

@nat-henderson
Copy link
Contributor

I've spread some comments around the child PRs - I think that there are a ton of format issues that could use addressing, while we're doing a giant sweeping format PR. If you say "nah, that stuff sounds like a hassle", I get it - merge 'em and maybe someday we'll come back for another go.

@nat-henderson
Copy link
Contributor

(I say "we're doing a giant sweeping format PR", but of course I mean "you're doing" - this is absolutely fantastic and will save me hours of swearing at the compiler)

rambleraptor and others added 3 commits July 18, 2018 12:23
Tracked submodules are build/puppet/compute build/puppet/sql build/puppet/storage build/puppet/container build/puppet/dns build/puppet/pubsub build/puppet/resourcemanager build/chef/compute build/chef/sql build/chef/storage build/chef/container build/chef/dns build/terraform build/ansible.
@modular-magician modular-magician merged commit d23cfa3 into master Jul 18, 2018
@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 51ffc6b) have been included in your existing downstream PRs.

chrisst pushed a commit to chrisst/magic-modules that referenced this pull request Oct 26, 2018
* Remove redundant '(Computed)' text

* Add support for setting labels on compute_disk

* Fix minor spacing issue in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants