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

Add the framework for package-specific settings. #196

Merged
1 commit merged into from
Mar 13, 2018

Conversation

zvezdan
Copy link
Member

@zvezdan zvezdan commented Mar 10, 2018

Add the interface and the default implementation for package specific
settings. Use the framework calls in pip install and build wheel tasks.

@zvezdan zvezdan requested a review from a user March 10, 2018 04:03
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good. The only change I want is to not mutate the origonal map.

testProjectDir.buildFile << """\
| plugins {
| id 'com.linkedin.python-pex'
| }
| version = '1.0.0'
| version = '1.0.0-MPDEP'
Copy link

Choose a reason for hiding this comment

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

This should be SNAPSHOT vs MPDEP.

Map<String, String> environment

@Input
@Optional
Copy link

Choose a reason for hiding this comment

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

Why is it optional? It has a default, would it be required?

public class DefaultEnvironmentMerger implements EnvironmentMerger {
@Override
public Map<String, String> mergeEnvironment(Map<String, String> target, Map<String, String> source) {
if (source != null) {
Copy link

Choose a reason for hiding this comment

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

I think you probably want to make a new map, and insert all of the non-null values into it. This will change the origional I feel will cause problems down the road.

*
* @param <T> Type of the object that represents package information.
*/
public interface PackageSettings<T> {
Copy link

Choose a reason for hiding this comment

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

Do we want to bound T to be something?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent is to have a generic type for future development. No need to change T in this interface when we change the package information type.

@zvezdan zvezdan force-pushed the package-specific-settings branch from 154094e to 9ec4b68 Compare March 13, 2018 00:51
Add the interface and the default implementation for package specific
settings. Use the framework calls in pip install and build wheel tasks.
@zvezdan zvezdan force-pushed the package-specific-settings branch from 9ec4b68 to 3da7f6e Compare March 13, 2018 02:13
@ghost ghost merged commit 987e0ca into linkedin:master Mar 13, 2018
@zvezdan zvezdan deleted the package-specific-settings branch March 13, 2018 20:15
This pull request was closed.
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.

1 participant