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.

Sign up for GitHub

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

Jump to bottom

Expose to_<str> as a public function #68965

Merged
merged 3 commits into from Apr 16, 2020
Merged

Expose to_<str> as a public function #68965

merged 3 commits into from Apr 16, 2020

Conversation

jborean93
Copy link
Contributor

SUMMARY

These functions are used throughout Ansible and now in different collections and having it behind a "private" import may put people off from using them. The functions haven't changed dramatically recently and I think it's time we expose them properly.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module_utils.text.converters

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 15, 2020
@bcoca
Copy link
Member

bcoca commented Apr 15, 2020

now only 1200+ refs to change ...

@jborean93
Copy link
Contributor Author

How many collections as well :)

@ansibot ansibot added the test This PR relates to tests. label Apr 15, 2020
webknjaz
webknjaz requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Almost there. But a few things are missing

lib/ansible/module_utils/_text.py Outdated Show resolved Hide resolved
@jborean93 jborean93 merged commit 79fff7d into ansible:devel Apr 16, 2020
@jborean93 jborean93 deleted the to_text-public branch April 16, 2020 21:54
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Apr 17, 2020
@felixfontein
Copy link
Contributor

The old version of _text also exposed text_type and binary_type (from ansible.module_utils.six). There is at least one module in community.general which was using them. I'll fix that module, but there might be more modules/plugins using that.

@felixfontein felixfontein mentioned this pull request Apr 17, 2020
Merged
@abadger
Copy link
Contributor

abadger commented Apr 17, 2020

@jborean93 note: samdoran had a pr that did this and also changed imports to use the new location. The import change was performed vian the rope refactoring library. Might want to get that info from him and update the imports when he's back

abadger
abadger reviewed Apr 17, 2020
View reviewed changes
lib/ansible/module_utils/common/text/converters.py
if PY3:
to_native = to_text
else:
to_native = to_bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put these first in the file. The callees should be defined before the callers by convention.

Copy link
Contributor Author

@jborean93 jborean93 Apr 17, 2020 •
edited

Choose a reason for hiding this comment

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

I just copied what was exactly in the _text.py, didn't want to really change the contents up. Also does that work without to_text or to_bytes being defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, what i meant was that the container_to_* functions call to_text/to_bytes so all of these moved functions should be at the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no worries, will raise a new PR next week to fix this and the missing imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the functions to the top of the file in #69090.

@jborean93 jborean93 mentioned this pull request Apr 22, 2020
Merged
@jborean93
Copy link
Contributor Author

@felixfontein I've added the six imports back in with #69090.

bcoca pushed a commit to bcoca/ansible that referenced this pull request Apr 22, 2020
@jborean93 @bcoca
Expose to_<str> as a public function (ansible#68965)
af50258 
* Expose to_<str> as a public function

* Fix sanity checks

* Move docstring to start of util
bcoca pushed a commit to bcoca/ansible that referenced this pull request Apr 28, 2020
@jborean93 @bcoca
Expose to_<str> as a public function (ansible#68965)
512667e 
* Expose to_<str> as a public function

* Fix sanity checks

* Move docstring to start of util
@ansible ansible locked and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel left review comments

@abadger abadger abadger left review comments

@webknjaz webknjaz webknjaz approved these changes

@bcoca bcoca bcoca approved these changes

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jborean93 @bcoca @felixfontein @abadger @sivel @webknjaz @mkrizek @ansibot