#270 Port to python3: inventory-qcow2, inventory-rpm and inventory-local
Merged by astepano. Opened by ssahani.
ssahani/standard-test-roles port-python3-2  into  master

Download 270.patch

@ssahani, could you explain this change please?

This function in original python 2 script has an optional parameter: path
This function in python 3 script has one required positional parameter: path

You completely change function specification.

Before function could be called in 3 different ways:

  1. get_artifact_path()
  2. get_artifact_path(path="/path")
  3. get_artifact_path("/path")

With this change you forbid to use methods 1 and 2.

I asked you to revert your changes for the same place at https://pagure.io/standard-test-roles/pull-request/265#_1,34

Please check : commits history: https://pagure.io/standard-test-roles/commits/master

If I marge your PR what commit comment will look like? It will be:

Merge 270 Port to python3: another set

Please change this comment to appropriate meaningful form, that will help read git history.

Before function could be called in 3 different ways:

get_artifact_path()
get_artifact_path(path="/path")
get_artifact_path("/path")

Let's see the usage of the function how this is used.

'log_file = get_artifact_path(LOG_FILE) '
Now where you plan to use it with a black value.
option 2 and 3 still works.

Please check : commits history: https://pagure.io/standard-test-roles/commits/master
If I marge your PR what commit comment will look like? It will be:
Merge 270 Port to python3: another set
Please change this comment to appropriate meaningful form, that will help read git history.

you sure Note: it contains 3 PRS but yes should be properly should be named

  1. @ssahani, you have a very clear task: "port scripts to Python 3". Please go and read your Jira ticket.

  2. In this PR you have changed function specification. This is unrelated to Python3 porting.
    In your PR comment you say: "This is about python3 porting". But, it is not.

  3. I am trying to review your PR according to your PR comment. Please explain, how this change to function specification is related to Python3 porting?

@bgoncalv @mvadkert @bookwar please review this RP.

@ssahani , please write comments to your PR in readable format.

you sure Note: it contains 3 PRS but yes should be properly should be named

Above comment very hard to read and understand. Please add punctuation marks. Is this statement or question? Please save our time.

@ssahani, you have a very clear task: "port scripts to Python 3". Please go and read your Jira ticket.

Please don't try to make me understand what is my task.

In this PR you have changed function specification. This is unrelated to Python3 porting.
In your PR comment you say: "This is about python3 porting". But, it is not.

your function specification does not have any use case.

If I marge your PR what commit comment will look like? It will be:
it's not marge it's merge. Please fix your spelling first

If I marge your PR what commit comment will look like? It will be:
it's not marge it's merge. Please fix your spelling first

You are right, sorry.

@ssahani could you just revert the change of get_artifact_path ? It has nothing to do with python3 porting. A part from it everything else looks good.

rebased onto 4b7ced6e1e2581a1d52d889212cd260bfd7ec08b

updated

Looks good, merge. Thank you.

Commit f196ed6f fixes this pull-request

Pull-Request has been merged by astepano

Pull-Request has been merged by astepano

Metadata