Python 3 compatibility and flake8

[ocefpaf]

@lsetiawan do not merge this yet. Let's test it with a real case deployment first.

There is a lot in this PR but very little are real code changes. For example:

• I replaced the use of some builtins as variable to avoid shadowing them; there are more but I did not touch them b/c it would change the API, like the use of type in some methods args;
• there are many comments and commented out code with triple quotes; those will be interpreted as doctrings by sphinx and we need to either removed them or use #;
• remove a few import * but that could break the code is a shadowed module is expected to be present;
• removed a test that we unrelated to the package.

[lsetiawan]

Wow that is a lot! I will take a look at this in a bit. Thanks @ocefpaf. 😄

[ocefpaf]

Wow that is a lot!

Sorry about that :grimacing: there is no way to actually review this line-by-line. The best approach is to use this branch for a real case scenario and see of that works.

[emiliom]

Thanks, @ocefpaf!! As I mentioned in my email to you just now, BiG CZ project priorities for the next 5 weeks will be a project-end "user" workshop we're putting together. Because of that, Python 3 compatibility will be low on the priorities for the time being, unfortunately.

Some specific comments:

I replaced the use of some builtins as variable to avoid shadowing them;

Great!

there are more but I did not touch them b/c it would change the API, like the use of type in some methods args;

This sounds like a bigger problem that we should expose via an issue. Could you create one to list the cases where "type" is used as methods args?

there are many comments and commented out code with triple quotes; those will be interpreted as doctrings by sphinx and we need to either removed them or use #;

I think I'd be conservative for now, and avoid removing comments. So, change to use # unless we (meaning @lsetiawan :wink:) are confident they're useless.

remove a few import * but that could break the code is a shadowed module is expected to be present;

Yeah, that's a bad practice that should be eliminated eventually. But we need to be very careful not to break something unexpected.

removed a test that we unrelated to the package.

Thanks.

[ocefpaf]

Because of that, Python 3 compatibility will be low on the priorities for the time being, unfortunately.

The Python 3 compatibility in this PR is actually a bonus. Most of the code changes are to implement better practices, avoid some confusions and bugs with builtins, import *, etc.

This sounds like a bigger problem that we should expose via an issue. Could you create one to list the cases where "type" is used as methods args?

Sure. I'll open one in a moment.

I think I'd be conservative for now, and avoid removing comments. So, change to use # unless we (meaning @lsetiawan :wink:) are confident they're useless.

I understand your point of view but when using git there is really no need for leaving commented code behind. It only leads to confusion, obfuscation and makes it really hard for third party coders to contribute to the code base. I guarantee that not even the person who left them there knows what it means :smile:

With that said, the commented out code will be preserve in the commit hashes before this one anyway. We can always find them with a quick GitHub search.

BTW , we should always try to open actionable issues instead of leaving commented out code in the code base.

But we need to be very careful not to break something unexpected.

The direct impact is tested via the flake8 linter. The indirect impact is impossible to foresee an, like if someone imports X and expects Y to be there b/c we had an implicit import in X. Only production tests will reveals cases like this.

[lsetiawan]

I think I'd be conservative for now, and avoid removing comments. So, change to use # unless we (meaning @lsetiawan 😉) are confident they're useless.

I agree with @emiliom. I haven't used the majority of the code yet. I will evaluate them and remove the comments as I see fit as I use odm2api.

[emiliom]

I think I'd be conservative for now, and avoid removing comments. So, change to use # unless we (meaning @lsetiawan :wink:) are confident they're useless.

I understand your point of view but when using git there is really no need for leaving commented code behind. It only leads to confusion, obfuscation and makes it really hard for third party coders to contribute to the code base. I guarantee that not even the person who left them there knows what it means

One of my main reasons is to defer to Stephanie for those decisions. That means I'd prefer to wait until she's back and she can have a vote (even as we try to persuade her!).

I agree with @emiliom. I haven't used the majority of the code yet.

That's another reason.

I will evaluate them and remove the comments as I see fit as I use odm2api.

Yeah. If you see something that seems like it can obviously be removed, go ahead and do it (as we've done in some cases).

remove a few import * but that could break the code is a shadowed module is expected to be present;

The direct impact is tested via the flake8 linter. The indirect impact is impossible to foresee an, like if someone imports X and expects Y to be there b/c we had an implicit import in X. Only production tests will reveals cases like this.

By direct impact you mean within odm2api proper, right?

I'm undecided about how concerned (and conservative) to be about indirect impacts on current usage of odm2api ... Of course, this will only impact future odm2api releases, so we could say others can pin to older releases (including the last one) to be 100% safe. Hmm.

@ocefpaf, thanks for all the other comments. Sounds great.

[ocefpaf]

By direct impact you mean within odm2api proper, right?

No I mean the use on the module that had the import * because flake8 will trigger an error if anything is undefined and I fixed all those. We cannot foresee the use of an implicit import in another module. (By module I mean each .py file in the odm2api package.)

[lsetiawan]

@ocefpaf I have tested these changes against the EnviroDIY Postgresql Database and Little Bear River MySQL Database, there doesn't seem to be any breakage. Also travis are passing! So I think these changes are save to merge.

[lsetiawan]

@emiliom Okay if I merge or wait for Stephanie to review these also?

[ocefpaf]

wait for Stephanie to review these also?

This can wait for @sreeder. There is no rush. But we must keep in mind that it will be hard to rebase if we do more changes to the code base b/c this PR touchs almost 100% of it.

[lsetiawan]

@ocefpaf This will not get messed up as I update docstrings correct?

[ocefpaf]

@ocefpaf This will not get messed up as I update docstrings correct?

It will be a hard rebase :smile: but if you are only going to work on the docstring we can live with that. Code changes would be even harder.

[lsetiawan]

It will be a hard rebase 😄 but if you are only going to work on the docstring we can live with that. Code changes would be even harder.

Sorry 😞. As I work on ODM2 REST API, I'll probably make changes to docstrings, and hopefully not have to change the code. If I have to make code changes, I'll just stash it somewhere until this PR is merged.

[emiliom]

I discussed this PR with @lsetiawan, and I'm comfortable going ahead and merging. It looks like it's ready to merge. I haven't read through every single comment, but I know you two have discussed it extensively.

Don, please merge at your convenience. I'll leave it up to you, but you have my vote :stuck_out_tongue_winking_eye:

[lsetiawan]

Current master is now in branch master_10102017

[emiliom]

Great, thanks.

[emiliom]

@lsetiawan, based on our discussion on Monday and you having created that "safe archive" branch, I think we decided we can move forward with merging this PR, right?

[lsetiawan]

See https://github.com/ODM2/ODM2PythonAPI/pull/111#issuecomment-337053581 for where the branch is. We are ready to go.

[emiliom]

Ok, merging! We'll blame @ocefpaf if anything goes wrong :smiling_imp: