dmwm / PHEDEX

CMS data-placement suite
8 stars 18 forks source link

Updated set and StringIO for easier porting to python3 #1043

Closed SaltumDis closed 7 years ago

SaltumDis commented 7 years ago

@ericvaandering mentioned that all projects will move to py3 one day. This is preparation for that.

nikmagini commented 7 years ago

Hi Adelina,

a few comments:

1) Please keep in mind that the python code in Documentation/WebSite/PlotConfig is importing modules from here:

https://github.com/nikmagini/webtools/tree/master/Tools/GraphTool

And this code is practically unsupported.

2) Are you sure that io and StringIO are the same in py2.7? Reading the docs, io is treating text as Unicode (like py3) while StringIO is using str. We should check that the change doesn't break anything (in particular in the underlying GraphTool modules). So I'm thinking to reject the changes to StringIO for now.

3) I'd suggest to keep different compatibiliy changes in different pull requests - can you please open separate requests for set and io?

Thanks! Nicolo'

SaltumDis commented 7 years ago

@nikmagini

  1. Should I make PR to https://github.com/nikmagini/webtools/tree/master/Tools/GraphTool ?
  2. They are different. StringIO.StringIO accept either Unicode or 8-bit strings. io.StringIO only accept Unicode. And yes, a bit of testing should be mandatory.
  3. Sure!
nikmagini commented 7 years ago

Closing, will reopen as separate PRs.

nikmagini commented 7 years ago

1) set built-in changed in https://github.com/dmwm/PHEDEX/pull/1053

2) To see what is needed, I also updated two scripts to use io instead of StringIO in https://github.com/dmwm/PHEDEX/pull/1054 Note that the migration StringIO --> io can NOT be done automatically, because depending on usecase StringIO.StringIO should be converted either to io.StringIO or io.BytesIO (e.g. pycurl requires bytes strings)

3) For the previous reason, I will not apply any StringIO changes to the website code - they should be done carefully only when (and IF) the website is ever going to be migrated to py3