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 copy() method to datatype #6269

Merged
merged 4 commits into from Aug 1, 2018

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 29, 2018

Problem

We have a lot of calls to the CCompiler, CppCompiler, and Linker constructors in native_toolchain.py to add extra cli arguments or environment variables, and we currently have to specify the value of every field of these constructors, even if we are only changing one or two of them.

Solution

  • add a copy(self, **kwargs) method to datatype which creates a new object with any entries from kwargs applied. this particular interface was inspired by scala case classes
  • add testing for the new copy() method
  • consume copy() in native_toolchain.py

Result

When creating datatype objects derived from existing objects of the same type, only the changed fields need to be specified, improving the readability of the code in native_toolchain.py.

cosmicexplorer added some commits Jul 29, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 29, 2018

Please verify the tests pass for util in both Py2 and Py3 by adding compatibility='CPython>=3.5', to the BUILD file entry for test_objects.py.

The folder should be green on master, so if you're seeing issues unrelated to your change then merge master.

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

this particular interface was inspired by the scala structs generated by scrooge

FTR: all case classes in Scala get a copy method.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 29, 2018

@Eric-Arellano:

13:09:36 05:41       [run]
                     ============== test session starts ===============
                     platform linux -- Python 3.6.6, pytest-3.6.4, py-1.5.4, pluggy-0.7.1
                     rootdir: /home/cosmicexplorer/tools/pants/.pants.d, inifile: /home/cosmicexplorer/tools/pants/.pants.d/test/pytest-prep/CPython-3.6.6/eaebe343317e70535f8d07a0c9a8a1e804c12d71/pytest.ini
                     plugins: cov-2.4.0, timeout-1.2.1
                     collected 36 items
                     
                     tests/python/pants_test/util/test_objects.py . [  2%]
                     ...................................        [100%]
                     
                      generated xml file: /home/cosmicexplorer/tools/pants/.pants.d/test/pytest/tests.python.pants_test.util.objects/junitxml/TEST-tests.python.pants_test.util.objects.xml 
                     =========== 36 passed in 1.15 seconds ============
                     
                   tests/python/pants_test/util:objects                                            .....   SUCCESS

This feels something we should test in CI soon.

@stuhood: updated the OP to reflect this very basic and useful information you have just provided to me.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 29, 2018

Thanks @cosmicexplorer!

That would be great if we could add to CI for regressions easily.

These are the folders that work in Py3 right now

  • src/ivy/
  • src/process/
  • src/releases/
  • src/reporting/
  • src/stats/
  • src/subsystem/
  • src/util/
  • test/tasks

If it would take more than 20-30 minutes to set up, however, I don't think it's worth it, because we're fairly close to getting all CI to be green in Py3. Barring the Rust ImportError issue, I'm expecting to finish this stage by the end of the week or early next week. At that point, we can switch CI to running fully in Py2 + Py3.

A simpler workaround in meantime is for me to ask reviewers to test locally. I made this script to add Compatibility to every entry in BUILD file and then run tests in Py3, so for example you would say ./run_py3.py util to run the whole util folder in Py3 mode.

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Jul 29, 2018

@@ -164,6 +159,11 @@ def __str__(self):
class_name=type(self).__name__,
typed_tagged_elements=', '.join(elements_formatted))

def copy(self, **kwargs):

This comment has been minimized.

@jsirois

jsirois Jul 30, 2018

Member

The _replace method above is a non-typesafe api-copy of copy from namedtuple. It has no production callers - can it be killed? If not, can it just be an alias for _replace or vice versa?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 30, 2018

Contributor

I addressed this in 5c13575! Is it considered to be breaking the API if we now raise a TypeError on a field name that doesn't exist, instead of the ValueError of the previous _replace() implementation? I would think not, but I'm not sure.

This comment has been minimized.

@jsirois

jsirois Jul 30, 2018

Member

It would be a debatable break since that part of the API was not documented. The datatype function is not marked :API: public though (see: https://www.pantsbuild.org/deprecation_policy.html), so this is technically moot.

@stuhood

stuhood approved these changes Aug 1, 2018

@stuhood stuhood merged commit 89c6b40 into pantsbuild:master Aug 1, 2018

1 check passed

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

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Add copy() method to datatype (pantsbuild#6269)
### Problem

We have a lot of calls to the `CCompiler`, `CppCompiler`, and `Linker` constructors in `native_toolchain.py` to add extra cli arguments or environment variables, and we currently have to specify the value of every field of these constructors, even if we are only changing one or two of them.

### Solution

- add a `copy(self, **kwargs)` method to `datatype` which creates a new object with any entries from `kwargs` applied. *this particular interface was inspired by scala case classes*
- add testing for the new `copy()` method
- consume `copy()` in `native_toolchain.py`

### Result

When creating datatype objects derived from existing objects of the same type, only the changed fields need to be specified, improving the readability of the code in `native_toolchain.py`.

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Sep 1, 2018

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