Skip to content

Conversation

@xianyuanjia
Copy link
Collaborator

@xianyuanjia xianyuanjia commented Oct 8, 2019

This change is Reviewable

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia)


mobly/base_test.py, line 76 at r1 (raw file):

        log_path: string, specifies the root directory for all logs written
            by a test run.
        testbed_name: string, the name of the test bed used by a test run.

you shouldn't directly rename a public attribute.
follow the standard SW release practice.

Add the new name, mark the old name deprecated (not removal), and wait for the next major version bump to remove the deprecated one.
See my recent PR to remove deprecated APIs.

Copy link
Collaborator Author

@xianyuanjia xianyuanjia left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia and @xpconanfan)


mobly/base_test.py, line 76 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

you shouldn't directly rename a public attribute.
follow the standard SW release practice.

Add the new name, mark the old name deprecated (not removal), and wait for the next major version bump to remove the deprecated one.
See my recent PR to remove deprecated APIs.

Done.

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @xianyuanjia)

@xpconanfan xpconanfan added this to the Mobly Release 1.10 milestone Oct 8, 2019
Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia)


mobly/config_parser.py, line 165 at r2 (raw file):

        log_path: string, specifies the root directory for all logs written by
            a test run.
        testbed_name: string, the name of the test bed used by a test run.

this is also a public API, which cannot be simply renamed

Copy link
Collaborator Author

@xianyuanjia xianyuanjia left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia and @xpconanfan)


mobly/config_parser.py, line 165 at r2 (raw file):

Previously, xpconanfan (Ang Li) wrote…

this is also a public API, which cannot be simply renamed

Done.

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia)


mobly/base_test.py, line 78 at r3 (raw file):

        test_bed_name: [Deprecated, use 'testbed_name' instead]
            string, the name of the test bed used by a test run.
        testbed_name: string, the name of the test bed used by a test run.

do you think we should move testbed_name into current_test_info?
(or just remove it since i can't find any usage of it...)

Copy link
Collaborator Author

@xianyuanjia xianyuanjia left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @xianyuanjia and @xpconanfan)


mobly/base_test.py, line 78 at r3 (raw file):

Previously, xpconanfan (Ang Li) wrote…

do you think we should move testbed_name into current_test_info?
(or just remove it since i can't find any usage of it...)

This is being used by certain power tests for logging purposes. I don't think this should be moved into current_test_info , since it is static and does not depend on the test case/stage being run.

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @xianyuanjia)

@xpconanfan xpconanfan merged commit 7ecec7d into google:master Oct 9, 2019
xianyuanjia added a commit to xianyuanjia/mobly that referenced this pull request Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants