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

Replace proxy settings with an enum. #135

Merged
merged 4 commits into from
Jan 19, 2021
Merged

Replace proxy settings with an enum. #135

merged 4 commits into from
Jan 19, 2021

Conversation

deepy
Copy link
Member

@deepy deepy commented Jan 19, 2021

This replaces the proxy setting with an enum containing:

  • SMART (previous true)
  • FORCED (always apply)
  • OFF (previous false)

@deepy deepy requested a review from bsautel January 19, 2021 11:17
@bsautel
Copy link
Contributor

bsautel commented Jan 19, 2021

That's good!

I improved the proxy documentation to explain the three different modes.

I am also wondering if we don't have a naming issue. The property is named useGradleProxySettings and the enum ProxySetting. Should not the enum end with an s? My english is quite poor and I am not sure about that, but it sounds like the two names should be consistent. What do you think about that?

Some integration tests are broken, but it sounds like it is not because of those changes, it's because of the Node.js upgrade and an remaining hardcoded npm version that I fixed in the master branch.

@deepy
Copy link
Member Author

deepy commented Jan 19, 2021

Ooh, good catch, and we should probably rename useGradleProxySettings to something else as well.
Maybe inheritGradleProxySettings or nodeProxySettings

@bsautel
Copy link
Contributor

bsautel commented Jan 19, 2021

Ok for renaming the property. I would say nodeProxySettings or even proxySettings.

Copy link
Contributor

@bsautel bsautel left a comment

Choose a reason for hiding this comment

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

That looks very good!

@deepy deepy merged commit e6e1f5f into master Jan 19, 2021
@deepy deepy deleted the proxy-settings-enum branch January 19, 2021 23:16
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