Skip to content

Some Rubocop fixes#4595

Merged
philippthun merged 5 commits intomainfrom
rubocop-fixes
Oct 9, 2025
Merged

Some Rubocop fixes#4595
philippthun merged 5 commits intomainfrom
rubocop-fixes

Conversation

@moleske
Copy link
Copy Markdown
Member

@moleske moleske commented Oct 7, 2025

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:
    fix the easy rubocop issues

  • An explanation of the use cases your change solves
    less output when running rubocop

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@moleske moleske requested review from a team October 7, 2025 00:05
end

def create_seed_security_groups(config)
return unless config.get(:security_group_definitions) && SecurityGroup.count == 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubocop gave a bad advice here. On a model/dataset we should use empty? as this issues an optimized SQL query. The none? function is implemented in Enumerable, thus all entries are fetched to check if there is any. count and empty? are Sequel functions instead:

.count == 0  =>  SELECT count(*) AS "count" FROM "security_groups" LIMIT 1

.none?       =>  SELECT * FROM "security_groups"

.empty?      =>  SELECT 1 AS\"one" FROM "security_groups" LIMIT 1

The other good function besides empty? is any? as we use the any_not_empty extension. All other Enumerable functions should be used with care on a model/dataset.

where(Sequel.lit("run_at < CURRENT_TIMESTAMP - INTERVAL '?' DAY", force_delete_after))

unless orphaned_delayed_jobs.count.zero?
unless orphaned_delayed_jobs.none?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be changed to if orphaned_delayed_jobs.any?.


started_app_count = ProcessModel.where(state: 'STARTED').count

expect(AppUsageEvent.count > 1).to be true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

many? also issues a SELECT * FROM ... - so here count seems to be the better option.

end

def validate_remove_billing_manager_by_guid!(org)
return if org.billing_managers_dataset.count > 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

many? and one? also issue a SELECT * FROM ... - so here count seems to be the better option.

@philippthun
Copy link
Copy Markdown
Member

Actually I like using many? and one? and none? as they are more verbose end expressive. I'm wondering why Sequel does not override them... Maybe we should do this? Then the code would look cleaner and we would not have to care about accidentally using the wrong function...

@moleske
Copy link
Copy Markdown
Member Author

moleske commented Oct 8, 2025

Actually I like using many? and one? and none? as they are more verbose end expressive. I'm wondering why Sequel does not override them... Maybe we should do this? Then the code would look cleaner and we would not have to care about accidentally using the wrong function...

Would you prefer I remove the commit for this Style/CollectionQuerying rubocop from this pr then? Then we can spend some time figuring out why Sequel doesn't override and decide if we want to override. Since the other rubocops seem fine might be worth doing that. Otherwise I can # rubocop:disable Style/CollectionQuerying on the lines you've marked in the review for count.

Also I definitely didn't think about what queries would actually be so thanks for calling that out

@philippthun
Copy link
Copy Markdown
Member

Would you prefer I remove the commit for this Style/CollectionQuerying rubocop from this pr then?

👍 I agree that this would be the easiest option for now.

@moleske
Copy link
Copy Markdown
Member Author

moleske commented Oct 9, 2025

Reverted, should be good to go now

@philippthun philippthun merged commit 2b5ac3a into main Oct 9, 2025
16 of 17 checks passed
ari-wg-gitbot added a commit to cloudfoundry/capi-release that referenced this pull request Oct 9, 2025
Changes in cloud_controller_ng:

- Some Rubocop fixes
    PR: cloudfoundry/cloud_controller_ng#4595
    Author: M. Oleske <moleske@users.noreply.github.com>
@moleske moleske deleted the rubocop-fixes branch October 9, 2025 15: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.

2 participants