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

Transform update_types in IgnoreCondition #3550

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

feelepxyz
Copy link
Contributor

@feelepxyz feelepxyz commented Apr 23, 2021

Moving the update_types transform from the config file parser to the
ignore condition making it easier to initialize IgnoreCondition without
fetching a config file.

Working towards being able to do this:

ignore_conditions = ignore_conditions.map do |ic|
  Dependabot::Config::IgnoreCondition.new(
    dependency_name: ic["dependency-name"],
    versions: [ic["version-requirement"]],
    update_types: ic["update-types"],
  )
end
Dependabot::Config::UpdateConfig.
  new(ignore_conditions: ignore_conditions).
  ignored_versions_for(dependency)

@feelepxyz feelepxyz requested a review from a team as a code owner April 23, 2021 12:54
@feelepxyz feelepxyz force-pushed the feelepxyz/ignore-conditon-type-transform branch from 030b2c6 to 1e2ae22 Compare April 23, 2021 13:06
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

LGTM: I'd assumed these exact strings only mattered when parsed directly from the config file.

If we're going to pass the verbatim strings through the external parser+storage and back into here, this works. I'd assumed it would be converted at some point, and intended to use the symbol.

What I really wanted is an enum in the JVM or an iota constant in Golang. Symbols where my guess at how Ruby might follow the same pattern...
With similar biases, I'd dig if these magic strings were defined and referenced as constants instead of duplicated across the codebase. The pattern in 7375421#diff-67a26681133898ab39f6dac25dad58b8bfa751fd7245ec723c196f1184f93101R9 seemed pretty neat!

common/lib/dependabot/config/ignore_condition.rb Outdated Show resolved Hide resolved
Moving the update_types transform from the config file parser to the
ignore condition making it easier to initialize IgnoreCondition without
fetching a config file.
@feelepxyz feelepxyz force-pushed the feelepxyz/ignore-conditon-type-transform branch from 1e2ae22 to 348d246 Compare April 23, 2021 13:27
@feelepxyz
Copy link
Contributor Author

If we're going to pass the verbatim strings through the external parser+storage and back into here, this works. I'd assumed it would be converted at some point, and intended to use the symbol.

It would be real nice to iterate towards moving all config file validation and parsing to core and using that code from api, this would make it easier to consolidate on transforms in one place. We could then fail jobs if the config is currently invalid which would increase visibility of these issues.

@feelepxyz feelepxyz merged commit 3ae9af0 into main Apr 23, 2021
@feelepxyz feelepxyz deleted the feelepxyz/ignore-conditon-type-transform branch April 23, 2021 13:48
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

LGTM! Just a nit about the transform.

end

private

def transformed_update_types
update_types.map(&:downcase).map(&:strip).compact
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
update_types.map(&:downcase).map(&:strip).compact
update_types.compact.map(&:downcase).map(&:strip)

Nit: this compact does nothing, as nil values have already raised?

irb(main):002:0> [nil].map(&:downcase)
(irb):2:in `map': undefined method `downcase' for nil:NilClass (NoMethodError)

I'm not sure if it should be refactored to be a guard for the map()s (the suggestion), or just removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Question: should this transform happen in the ConfigFile / parsing logic?

We might be able to simplify by assuming the caller of IgnoreCondition has normalized these values: either through ConfigFile or the mysterious server-side parsing process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Yeah true, might be better to move to the File class and rely on the api to have done this already

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.

None yet

2 participants