Skip to content
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

Fix the unquote function. #58725

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

@hobbestigrou
Copy link

@hobbestigrou hobbestigrou commented Jul 4, 2019

Check if the parameter is a string otherwise return the data. Modify the
documentation to add the type.

Copy link
Member

@sivel sivel left a comment

As I read over this, I feel like we should instead be modifying is_quoted instead of unquote.

Loading

lib/ansible/parsing/quoting.py Outdated Show resolved Hide resolved
Loading
lib/ansible/parsing/quoting.py Outdated Show resolved Hide resolved
Loading
@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Jul 9, 2019

Thanks for the review @sivel. However I'm not sure to understand what do you mean, do you want I rename also the function? If yes, I suppose we must stay compatible and first display a warning.

Loading

@ansibot
Copy link
Contributor

@ansibot ansibot commented Jul 9, 2019

The test ansible-test sanity --test use-compat-six [explain] failed with 1 error:

lib/ansible/parsing/quoting.py:20:1: use `ansible.module_utils.six` instead of `six`

click here for bot help

Loading

lib/ansible/parsing/quoting.py Outdated Show resolved Hide resolved
Loading
lib/ansible/parsing/quoting.py Outdated Show resolved Hide resolved
Loading
@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Jul 11, 2019

That seems good for you @sivel now?

Loading

@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Jul 18, 2019

Thanks @mattclay to add this? Do I must change something on my side? Locally all tests passed.

Loading

@mattclay
Copy link
Member

@mattclay mattclay commented Jul 18, 2019

Loading

@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Jul 19, 2019

The unit tests is fixed now.

Loading

Check if the parameter is a string otherwise return the data. Modify the
documentation to add the type.
Check the given path is a string and add modify the documentation.
Add test for the both functions.
:rtype: str
"""
if not isinstance(given, string_types):
return given
Copy link
Member

@sivel sivel Aug 5, 2019

Choose a reason for hiding this comment

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

In what case should a path not be a string? I question the validity of just returning what was given, if it wasn't a string. I might instead expect this to raise a TypeError instead.

Loading

@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Aug 6, 2019

Thanks for the review. It's fixed.

Loading

4383
4383 approved these changes Aug 6, 2019
Copy link
Contributor

@4383 4383 left a comment

LGTM

Loading

@ansibot ansibot removed the stale_ci label Aug 6, 2019
:rtype: str
"""
if not isinstance(given, string_types):
raise TypeError('The type of the given argument must be a string')
Copy link
Member

@sivel sivel Aug 6, 2019

Choose a reason for hiding this comment

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

Can you explain what led to needing this change? Where is a non string being passed into this function?

Loading

Copy link
Author

@hobbestigrou hobbestigrou Aug 19, 2019

Choose a reason for hiding this comment

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

Sorry I'm not sure to understand what do you mean, but it's just to be sure it's a string. Sorry for the delay of my answer, I'm going to be more proactive.

Loading

Copy link
Member

@sivel sivel Aug 19, 2019

Choose a reason for hiding this comment

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

This PR, per the description is "Fix the unquote function". As such, there is no explanation why a change was made to path_dwim

In what case is a non string being passed to the path_dwim method? What code path or behavior leads to this being necessary?

Loading

Copy link
Author

@hobbestigrou hobbestigrou Aug 20, 2019

Choose a reason for hiding this comment

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

If you want I can make this modification on another contribution. But I think it's good to check to type here also and fix the documentation.

Loading

Copy link
Member

@sivel sivel Aug 20, 2019

Choose a reason for hiding this comment

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

You still aren't answering my question.

In what case is a non string being passed to the path_dwim method? What code path or behavior leads to this being necessary?

Loading

Copy link
Author

@hobbestigrou hobbestigrou Aug 21, 2019

Choose a reason for hiding this comment

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

The same of the initial fix, a bug with ansible-lint. But I'm not sure to understand why it's important.

Loading

Copy link
Member

@sivel sivel Aug 21, 2019

Choose a reason for hiding this comment

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

While fixing bugs is important, we typically do not prioritize bugs that are the fault of other software, or software that misuses the Ansible Python API. Ansible itself does not suffer from these problems, thus it sounds like the actual bug is really in ansible-lint that is causing issues.

Making these changes, could result in bad behavior of Ansible itself, since we may be handling this differently elsewhere.

Loading

Copy link
Author

@hobbestigrou hobbestigrou Aug 21, 2019

Choose a reason for hiding this comment

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

Sorry you're right, I understand what you mean. So I can move this modification in another contribution or remove it simply, if you want and keep only the unquote function modification.

Loading

Raise an TypeError if the type of given argument is not a string.
@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Sep 6, 2019

Just to be sure, what do you want I make to have this patch merged? It's a real fix.

Loading

@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Oct 8, 2019

What I can modify there? I'm not sure to understand what I must make, to have this fix merge in master.

Loading

@hobbestigrou
Copy link
Author

@hobbestigrou hobbestigrou commented Oct 30, 2019

Any chance to have a merge or information about what i need to modify here.

Loading

@samdoran
Copy link
Member

@samdoran samdoran commented Mar 15, 2021

/azp run

Loading

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented Mar 15, 2021

Azure Pipelines successfully started running 1 pipeline(s).

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants