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 more configuration axis ScopeFilter factory methods #3994

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
3 participants
@fmlrt

fmlrt commented Mar 6, 2018

No description provided.

@eed3si9n eed3si9n added the ready label Mar 6, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Mar 6, 2018

Since this is a new feature, I would target 1.x.
Otherwise LGTM

@eed3si9n eed3si9n changed the base branch from 1.1.x to 1.x Mar 6, 2018

@fmlrt

This comment has been minimized.

fmlrt commented Mar 7, 2018

This PR originally targeted 1.1.x because it's amendment for lost feature.

In SBT 0.13.x one could do something like

Classpaths
  .interSort(projectRef, configuration, settings, buildDependencies)
   .map { case (projectRef, configurationName) =>
     ScopeFilter(inProjects(projectRef), inConfigurations(new Configuration(configurationName)))
   }

In SBT 1.x new Configuration is now illegal but not all places dealing with configurations use ConfigRef's, some still use strings.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Mar 7, 2018

From 1.1.1's point of view, I feel like this is a feature enhancement.
Plus hopefully this is something you can workaround if you really need it, correct?

@fmlrt

This comment has been minimized.

fmlrt commented Mar 7, 2018

Makes sense. I provided some context for changes since I forgot to add it right away.
In my particular case I found workaround - searching through all of project configurations and filtering them by name. Should do for now.

As a side note, it feels weird to have 3 ways to reference configuration - String, ConfigKey, ConfigRef. If I understand correctly ConfigRef should be only one eventually. But I found no issues related to that. @eed3si9n do you know if there are any?

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Mar 7, 2018

As a side note, it feels weird to have 3 ways to reference configuration - String, ConfigKey, ConfigRef. If I understand correctly ConfigRef should be only one eventually.

Feel free to open an issue for that. We should consolidate to ConfigRef.

@dwijnand

lgtm. thanks @fmlrt!

@dwijnand dwijnand added this to the 1.2.0 milestone Mar 8, 2018

@dwijnand dwijnand merged commit a85d760 into sbt:1.x Mar 8, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand removed the ready label Mar 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment