Python 3 compatibility and flake8 #111

ocefpaf · 2017-10-05T19:11:15Z

@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 · 2017-10-10T17:43:29Z

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

ocefpaf · 2017-10-10T17:47:56Z

Wow that is a lot!

Sorry about that 😬 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 · 2017-10-10T18:11:34Z

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 😉) 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 · 2017-10-10T18:33:23Z

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 😉) 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 😄

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 · 2017-10-10T18:33:53Z

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 · 2017-10-10T18:44:59Z

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 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 · 2017-10-10T18:48:07Z

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 · 2017-10-10T18:52:21Z

@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

LGTM! Tested with Postgres and MySQL.

lsetiawan · 2017-10-10T18:57:46Z

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

ocefpaf · 2017-10-10T18:59:37Z

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 · 2017-10-10T19:03:03Z

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

ocefpaf · 2017-10-10T19:07:45Z

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

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.

lsetiawan · 2017-10-10T19:11:42Z

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.

ocefpaf · 2017-10-11T11:26:06Z

odm2api/ODM2/models.py

@@ -265,6 +265,14 @@ class SamplingFeatures(Base):
    """float: The elevation of the sampling feature in meters, or in the case of Specimen, 
              the elevation from where the SamplingFeature.Specimen was collected"""
    ElevationDatumCV = Column('elevationdatumcv', ForeignKey(CVElevationDatum.Name), index=True)
+    """str: The code for the vertical geodetic datum that specifies the zero point for 


@lsetiawan are these suppose to be docstrings? Should we consolidate them into a single docstring or make them a comment with a #?

The auto-generated docs from this will be quite messy 😕

@ocefpaf I merely followed the google example:

self.attr3 = param3 #: Doc comment *inline* with attribute #: list of str: Doc comment *before* attribute, with type specified self.attr4 = ['attr4'] self.attr5 = None """str: Docstring *after* attribute, with type specified."""

There are three options and I did the last option. What do you think I should do? Thanks.

It is not about the example, it is about how we want the docs to look. Some people are on the side that no docs are better than bad automated docs. It is easy to create an automated doc page that is incomprehensible.

I am not saying that is the case here but someone should read the results with care to see if they make sense.

ocefpaf · 2017-10-11T11:26:36Z

odm2api/ODM2/services/readService.py

-        * Pass a SamplingFeature Well Known Text - return a list of sampling feature objects
-        * Pass whether or not you want to return only the sampling features
-          that have results associated with them
+        """Retrieve a list of Sampling Feature objects.


Nice one @lsetiawan! Just rebased from your PR. Can you take a look to see if everything is here?

Looks all good. Thanks!

emiliom · 2017-10-11T20:50:16Z

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 😜

lsetiawan · 2017-10-16T21:47:30Z

Current master is now in branch master_10102017

emiliom · 2017-10-16T21:50:42Z

Great, thanks.

emiliom · 2017-10-18T19:39:54Z

@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 · 2017-10-18T20:02:15Z

See #111 (comment) for where the branch is. We are ready to go.

emiliom · 2017-10-18T20:47:54Z

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

Python 3 compat and flake8

d449d47

ocefpaf requested a review from lsetiawan October 5, 2017 19:11

ocefpaf mentioned this pull request Oct 10, 2017

Avoid the use of builtins #115

Closed

lsetiawan approved these changes Oct 10, 2017

View reviewed changes

Merge branch 'master' into py3k

19f5f2f

ocefpaf commented Oct 11, 2017

View reviewed changes

lsetiawan mentioned this pull request Oct 11, 2017

Docstring Conventions #106

Open

5 tasks

emiliom merged commit 1cdf2e2 into ODM2:master Oct 18, 2017

ocefpaf deleted the py3k branch October 21, 2017 17:39

lsetiawan mentioned this pull request Jan 8, 2018

Issue new release? #135

Closed

aufdenkampe mentioned this pull request Feb 6, 2019

Transitioning to Python 3 ODM2/odm2-software-ecosystem#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3 compatibility and flake8 #111

Python 3 compatibility and flake8 #111

ocefpaf commented Oct 5, 2017

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

emiliom commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

emiliom commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

lsetiawan left a comment

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

ocefpaf Oct 11, 2017

lsetiawan Oct 11, 2017

ocefpaf Oct 11, 2017

ocefpaf Oct 11, 2017

lsetiawan Oct 11, 2017

emiliom commented Oct 11, 2017

lsetiawan commented Oct 16, 2017

emiliom commented Oct 16, 2017

emiliom commented Oct 18, 2017

lsetiawan commented Oct 18, 2017 •

edited

Loading

emiliom commented Oct 18, 2017

Python 3 compatibility and flake8 #111

Python 3 compatibility and flake8 #111

Conversation

ocefpaf commented Oct 5, 2017

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

emiliom commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

emiliom commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

lsetiawan left a comment

Choose a reason for hiding this comment

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

ocefpaf commented Oct 10, 2017

lsetiawan commented Oct 10, 2017

ocefpaf Oct 11, 2017

Choose a reason for hiding this comment

lsetiawan Oct 11, 2017

Choose a reason for hiding this comment

ocefpaf Oct 11, 2017

Choose a reason for hiding this comment

ocefpaf Oct 11, 2017

Choose a reason for hiding this comment

lsetiawan Oct 11, 2017

Choose a reason for hiding this comment

emiliom commented Oct 11, 2017

lsetiawan commented Oct 16, 2017

emiliom commented Oct 16, 2017

emiliom commented Oct 18, 2017

lsetiawan commented Oct 18, 2017 • edited Loading

emiliom commented Oct 18, 2017

lsetiawan commented Oct 18, 2017 •

edited

Loading