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

KB-H057 - os.rename is not recommended #294

Merged
merged 6 commits into from
Aug 3, 2021

Conversation

uilianries
Copy link
Member

The os.rename is no longer recommended, instead, use tools.rename

This PR adds this rule as warning, otherwise CCI will break heavily.

Related to #293

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries closed this Apr 28, 2021
@uilianries uilianries reopened this Apr 28, 2021
@jgsogo
Copy link
Contributor

jgsogo commented Apr 29, 2021

Right now this is probably the best approach to implement it... but in order to merge it and make it useful, we need to add some functionality to the library https://github.com/conan-io/c3i_jenkins/issues/758.

Let's wait a little bit, see how we implement it in the library before merging this one, maybe we come up with some changes to improve it.

@jgsogo jgsogo added this to the conan 1.37 milestone Apr 29, 2021
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

We will suggest user to use the new (to come) rename function that will let us configure the usage of robocopy: conan-io/conan#8897

We will update this PR once the functionality is ready to be used.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

Done, now the recommended way is tools.rename(self, src, dst)

@Croydon
Copy link
Contributor

Croydon commented Jun 14, 2021

tools.rename(self, src, dst) does not seem to be documented right now. Also why does it need the self reference?

@uilianries
Copy link
Member Author

@Croydon It's documented here: https://docs.conan.io/en/latest/reference/conanfile/tools/files.html#conan-tools-rename

The feature issue is this one: conan-io/conan#8897
But the origin of that topic is this one: conan-io/conan#7105

The self is part of remodeling tools module. It has been worked as 'utils', where we putted anything there and consumed as external package. Now, accessing self, we will keep the same pattern for for those methods. For instance, cross-building method consumes settings, but now we can use self, instead.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 15, 2021

I would add the full import chain to the error message: "... use conan.tools.rename(self, ...." otherwise people might not be aware of the new tool. It's not just about adding self as the first argument.

@Croydon
Copy link
Contributor

Croydon commented Jun 15, 2021

@jgsogo This was my confusion as I looked in the wrong place in the documentation

Signed-off-by: Uilian Ries <uilianries@gmail.com>
prince-chrismc
prince-chrismc previously approved these changes Jun 19, 2021
danimtb
danimtb previously approved these changes Jun 22, 2021
@danimtb
Copy link
Member

danimtb commented Jun 22, 2021

We need Conan 1.37.0 in the CCI pipelines at least before merging this

@uilianries uilianries dismissed stale reviews from danimtb and prince-chrismc via 3672054 June 22, 2021 18:37
@SSE4
Copy link
Contributor

SSE4 commented Jul 30, 2021

1.37 is already in CCI, right?

@uilianries
Copy link
Member Author

Yes, 1.37.2 is running on CCI right now! /cc @danimtb

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Time to approve this one 🚀

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

Successfully merging this pull request may close these issues.

6 participants