Deprecate `BuildFileAddress.build_file` #4511

Merged
merged 3 commits into from Apr 25, 2017

Conversation

Projects
None yet
5 participants
@stuhood
Member

stuhood commented Apr 24, 2017

Problem

BuildFileAddress.build_file (which is marked API: public) needs to go away, as BuildFile and BuildFileAddressMapper (which are not marked public) will be going away along with the v1 engine.

Solution

As part of #4365, switch all internal usages of BuildFileAddress.build_file to BuildFileAddress.rel_path, which is stringly typed, and sufficient for all known usages.

@kwlzn

kwlzn approved these changes Apr 24, 2017

lgtm!

src/python/pants/build_graph/address.py
def build_file(self):
"""The build file that contains the object this address points to.
- :API: public

This comment has been minimized.

@kwlzn

kwlzn Apr 24, 2017

Member

should this stay marked :API: public until it's removal?

technically, it's still public - just deprecated - until it's removed entirely.

@kwlzn

kwlzn Apr 24, 2017

Member

should this stay marked :API: public until it's removal?

technically, it's still public - just deprecated - until it's removed entirely.

@baroquebobcat

lgtm!

@JieGhost

Looks good! 1 nit.

def is_safe(t):
- return hasattr(t, 'address') and hasattr(t.address, 'build_file')
- return set(t.address.build_file.parent_path for t in targets if is_safe(t))
+ return hasattr(t, 'address') and hasattr(t.address, 'rel_path')

This comment has been minimized.

@JieGhost

JieGhost Apr 24, 2017

Contributor

nit: I remember in one of my previous reviews, you suggest using "getattr" instead of "hasattr". I don't quite remember the reasoning behind it, but just wonder if it applies here.

@JieGhost

JieGhost Apr 24, 2017

Contributor

nit: I remember in one of my previous reviews, you suggest using "getattr" instead of "hasattr". I don't quite remember the reasoning behind it, but just wonder if it applies here.

This comment has been minimized.

@stuhood

stuhood Apr 24, 2017

Member

Yea, generally that would be preferable. In this case, this is a filter on a list of things, so it would be awkward.

@stuhood

stuhood Apr 24, 2017

Member

Yea, generally that would be preferable. In this case, this is a filter on a list of things, so it would be awkward.

This comment has been minimized.

@JieGhost

JieGhost Apr 24, 2017

Contributor

I see.

@JieGhost

JieGhost Apr 24, 2017

Contributor

I see.

This comment has been minimized.

@stuhood

stuhood Apr 25, 2017

Member

The rust and scala collections libraries make this kind of thing much cleaner (with filter_map and collect, respectively), but yea... no great answer in python.

@stuhood

stuhood Apr 25, 2017

Member

The rust and scala collections libraries make this kind of thing much cleaner (with filter_map and collect, respectively), but yea... no great answer in python.

@stuhood stuhood merged commit bacf0ee into pantsbuild:master Apr 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/deprecate-address-dot-build_file branch Apr 25, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Deprecate `BuildFileAddress.build_file` (#4511)
### Problem

`BuildFileAddress.build_file` (which is marked `API: public`) needs to go away, as `BuildFile` and `BuildFileAddressMapper` (which are not marked public) will be going away along with the v1 engine.

### Solution

As part of #4365, switch all internal usages of `BuildFileAddress.build_file` to `BuildFileAddress.rel_path`, which is stringly typed, and sufficient for all known usages.

thesamet added a commit to thesamet/pants that referenced this pull request May 9, 2017

Deprecate `BuildFileAddress.build_file` (#4511)
### Problem

`BuildFileAddress.build_file` (which is marked `API: public`) needs to go away, as `BuildFile` and `BuildFileAddressMapper` (which are not marked public) will be going away along with the v1 engine.

### Solution

As part of #4365, switch all internal usages of `BuildFileAddress.build_file` to `BuildFileAddress.rel_path`, which is stringly typed, and sufficient for all known usages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment