cleans up imports, removes circular import

[a-tal]

• generally, the order is std lib, 3rd party, 1st party
• fixed line endings in a couple files
• random linting fixes
• adds package import tests

[Ebag333]

I had to add an empty __init__.py back to services to make pyCharm happy. It seems to run it, but it complains it can't find references to anything under services since the file got nuked.

Since you removed the third partying of __init__.py it's a lot clearer where the circular reference comes from.

  File "./pyfa.py", line 126, in <module>
    from gui.mainFrame import MainFrame
  File ".\gui\mainFrame.py", line 43, in <module>
    from gui.mainMenuBar import MainMenuBar
  File ".\gui\mainMenuBar.py", line 24, in <module>
    import gui.graphFrame
  File ".\gui\graphFrame.py", line 23, in <module>
    import gui.display
  File ".\gui\display.py", line 24, in <module>
    from gui.viewColumn import ViewColumn
  File ".\gui\viewColumn.py", line 65, in <module>
    from gui.builtinViewColumns import *
  File ".\gui\builtinViewColumns\misc.py", line 26, in <module>
    from service.fit import Fit, Market
  File ".\service\fit.py", line 36, in <module>
    from service.character import Character
  File ".\service\character.py", line 36, in <module>
    from service.fit import Fit
ImportError: cannot import name Fit

Not sure how to solve this, but tempted to just merge the files....

This doesn't work when patched against the cap sim branch I'm running into this issue on. Based on the stack trace, I'm 92% sure it won't work against master either.

Looking deeper at character.py the only place I can find that Fit is referenced (ignoring where it's passed into a function, since that's not the same thing even if its the same name...)

    def run(self):
        path = self.path
        sCharacter = Character.getInstance()
        sFit = Fit.getInstance()
        fit = sFit.getFit(self.activeFit)
        backupData = ""
        if self.saveFmt == "xml" or self.saveFmt == "emp":
            backupData = sCharacter.exportXml()
        else:
            backupData = sCharacter.exportText()

        if self.saveFmt == "emp":
            with gzip.open(path, mode='wb') as backupFile:
                backupFile.write(backupData)
        else:
            with open(path, mode='w',encoding='utf-8') as backupFile:
                backupFile.write(backupData)

        wx.CallAfter(self.callback)

The lines that use Fit don't seem to do anything. :/

Going to nuke and retest. Test seems to have worked, but not out of the woods yet!

In services.damagePattern.py this was nuked:

class ImportError(Exception):
     pass

It's used by patternEditor.py:

            try:
                sDP.importPatterns(text)
                self.stNotice.SetLabel("Patterns successfully imported from clipboard")
            except service.damagePattern.ImportError, e:
                self.stNotice.SetLabel(str(e))
            except Exception, e:
                self.stNotice.SetLabel("Could not import from clipboard: unknown errors")
            finally:
                self.entityEditor.refreshEntityList()

Same thing for service.targetResists.

Because service no longer imports stuff through the init, had to create a patch.

gangView.py
(This one has a few import references that we can probably get rid of. Like importing the market just so we can get a ship name.)

Screw it, too many files with 1 change. Massive patch file incoming....

Note to follow up on this:
For service.targetResists.py had to do lazy imports for import eos.db and import eos.saveddata.targetResists. There's probably a better way. Not sure why on eos.db, but on the other one it's because the class name would redefine the import statement. FML. Okay, found better way for targetResists (alias the import), not messing with DB right now.
Also targetResists.py under contextMenu has the same thing.

Under saveddata, targetResists.py has an error. Suspect that it is due to the class name redefining the import statement.

Commented out the import/export code that exists in service.fit.py. There's a circular import between that and service.port.py. We need to migrate those functions over to port.py (I'm guessing, or somewhere else, they can't be in fit.py.

[Ebag333]

Note to self to follow up on this.

In mainFrame.py there is this little bit of code:
"\n\nEVE Data: \t" + eos.config.gamedata_version +

Only problem is....there is no gamedata_versrion in eos.config.

[Ebag333]

So it took just a wee bit to get Pyfa to run again. But good news! Not only does it run, but I tested it again the Gnosis service code, no more import errors! \o/

@a-tal I think I did this right. If you merge the PR hopefully it gets updated here.
a-tal#1

Do you think you could re-go over the imports once you merge that (and confirm that this PR updates)?

.

@blitzmann couple lessons learned here, and a few things we need to do.

1. We have really bad shadowing. Not only do we shadow built in functions, but we shadow ourselves repeatedly. It would be nice if we had some sort of naming convention for classes that would prevent this, so we don't have 3 fit,py files, each with Fit() defined in them.
2. We redefine imports. I found at least 3 files where we imported from types.py then redefined the same name to the one from services. (from types import Fit with from services import Fit in the same file.) This is cleaned up for anything in services, but I'm sure there's more to be done. Which brings me to.....
3. types.py needs to DIAF.
4. There is a lot more cleanup that can be done. There's tons of unused imports, and a number of modules where we import something just to do 1 simple thing.
5. We need to move the export code out of service\fit.py. It doesn't belong there, and indeed cannot be in there due to circular imports. Would appreciate it if you helped with this, since you're much more familiar with the export code than I am. For now it's commented out, which means most of the export/import functions are disabled until we get it fixed.
6. We had at least two cyclical imports. I have no idea how this even worked. By all rights it should not have. I'm guessing that third partying it through __init__.py and types.py is what made it work, but who knows.....

[blitzmann]

Haven't read much of this yet because :verbosity and time:, but:

• What exactly are we trying to solve here? Is there a problem with something? I scanned very briefly, but don't see a summary of why? I am very hesitant to merge in low-level changes like this without a complete understanding of it's implications and reasons.
• types.py needs to DIAF. I love types.py. It contains a singular source to get all types that are sually needed. :(

[Ebag333]

What exactly are we trying to solve here? Is there a problem with something? I scanned very briefly, but don't see a summary of why? I am very hesitant to merge in low-level changes like this without a complete understanding of it's implications and reasons.

The problem is that we have cyclical imports. Here is an example after you strip away it being hidden in service\__init__.py

File ".\service\fit.py", line 36, in <module>
    from service.character import Character
  File ".\service\character.py", line 36, in <module>
    from service.fit import Fit
ImportError: cannot import name Fit

fit and character import each other. port and fit import each other. There's 1 more than was cyclical too, but I don't recall which one. And there's another unknown amount that were cyclical, but it's damn hard to tell how many because they did import type and import service which masks what they're actually importing.

The imports were so fragile that simple dropping gnosis.py into services broke them. Even though gnosis.py didn't reference anything (other than calling EVE_Gnosis). For reasons if I moved gnosis.py into it's own folder, it worked, which was fine as long as gnosis didn't need to reference anything. But we need the settings in there, which is a very legitimate use of that service, and should have no problems pulling in, right? Wrong, the whole house of cards came tumbling down.

types.py needs to DIAF. I love types.py. It contains a singular source to get all types that are usually needed. :(

usually in this case meant importing every single type (or many types) when only one or two are needed.

Take this code snippet for example (this is just psuedo code but it's uncomfortably close to real code):

import types # need to get some attributes
import service # oh I need settings

def Fit(self):
    self.fit = Fit() # register Fit into self so we can reference it later

Which Fit did you just execute? Did you execute Fit which is from types, and is actually eos.saveddata.fit? Or was it Fit from services which is actually services.fit? Or was it Fit defined by the function? Or maybe for kicks and giggles it's Fit which is defined in yet another module and imported locally onto that scope (example not shown in the psuedocode above).

In gui/builtinContextMenus/damagePattern.py this isn't theoretical, this is literally how it was done (except we're redefining DamagePattern as a local function instead of Fit). There was another module where I had to alias the import as well because of Fit though I don't recall which of them it was. It drove me nuts for about 3 minutes because I thought I was going insane since I kept flipping between the two common Fit imports used....then I realized that the module needed both eos.saveddata.fit and services.fit.

TBH, the current code should not work. And, it might not, depending on the platform, version of python, phase of the moon, etc. Between the cyclical imports, re-definitions, and shadowing, it's shocking to me that you can just do Fit....... and it somehow magically pulls from the right one.

Here's the reasons why we should merge this change (and continue burning this out with fire):

1. Because code that doesn't even touch the rest of the Pyfa code causes the whole import house of cards to come tumbling down it it's put in services. Wut?
2. Third partying the import by hiding it in one of the many __init__.py (or types) just masks what is actually being imported.
3. Because importing types (or even worse, putting massive imports in __init__.py) means getting imports that aren't needed, maybe don't want, and increases chances of cyclical imports. And just massively bloating the code because someone did import types when all they needed was one specific import, and not one of the other 41 imports. That's the actual number, 42 things in there that are referenced directly. Oh, and don't forget import eos.db which is also in there and references Bob-only-knows how much other stuff.
4. Because when we do from very.deep.module.i.need import * we get the imports off every __init__.py in each folder down the path, every time. Which is some more of 2 and 3, but also as a fun bonus means it's slowing things down. Fortunately we aren't too deep (3 layers I think is max), but it's still bad.
5. Because it redefines things all the time.
6. Because it breaks code inspection. As soon as the third partying was removed, a bunch of stuff lit up that wasn't right (like the two examples above).

Okay, bring solutions not problems.

1. Fix the one remaining cyclical import (ports and fit in services) by moving the export code that's commented out currently.
2. Unless someone spots something, once the PR I submitted to @a-tal gets merged into his master, it should update the PR here with my commit. Then we can merge this into Pyfa-org master. If something goes sideways I have a backup of the code at the state where all the imports are fixed and Pyfa is functional again. We might lose some git blame but I'd rather see the imports fixed than get credit.
3. More refactoring is needed. We have tons of imports that aren't used at all, and a bunch of pointing to types. A follow up PR should be done removing the masked imports through types. This will no doubt expose other things that shouldn't be done.
4. Because we love shadowing, come up with a convention for dealing with it. As an example we have four fit.py files, and two of them are used all over the place with the special guest appearance of the other two. So from <place> import Fit is bad because we don't know which Fit we're referencing. I would suggest (to avoid too much refactoring) that we simply alias every single import with a standard convention. For example we could do from eos.db.saveddata.fit import Fit as eds_Fit so A) we know which Fit it is and B) we won't hit 3 things if we just run Fit(). Another option would be to just do as import_Fit but the problem there is that there are a few places where we need two or more Fit objects. This would eliminate the shadowing we do of own code, though it wouldn't necessarily eliminate the shadowing we do of built in functions (that's another topic for another day).
5. Eliminate locally scoped imports. I think we got most of them, but I'm sure that there are other imports that aren't done at the module level, but hidden under Class (or perhaps even function) definitions. All the ones I could find are moved up to module level imports, so we're more standardized.

Finally, remember this?
https://files.slack.com/files-pri/T099289K7-F34UE6MT3/pasted_image_at_2016_11_22_12_49_am.png

A lot of that will get cleaned up, and it should make a lot more sense. Your profiler also probably won't hurrrrr as much. :)

And, with any luck, most of those self referencing loops will go away.

[Ebag333]

Took care of the exports. That was easy, there was literally no need for them to call fit.py in the first place.

The imports and backups are a bit harder. Those should also be moved to port.py, (at least 1 function has to, because of circular references).

[Ebag333]

Cleaning up the circular imports out of service, got the import/export stuff moved over from fit.py to port.py.

Now:

  File ".\eos\effectHandlerHelpers.py", line 24, in <module>
    from eos.saveddata.module import Module as es_Module
  File ".\eos\saveddata\module.py", line 25, in <module>
    from eos.effectHandlerHelpers import HandledItem, HandledCharge

le sigh

[a-tal]

you can now use this to check your code for obvious errors:

flake8 --ignore=E501,E731 service gui eos utils config.py pyfa.py

I'd highly recommend using that flake8 command (or pep8 if you prefer) in your CI somewhere

[blitzmann]

I keep seeing notifications about this PR. Is this finalized or not?

[Ebag333]

Not even close. 😣

[blitzmann]

What still needs to be done? Please provide a todo list and mark off what exactly has happened here along with what needs to be done. I want to keep abreast of it since this PR is quickly becoming unmanageable form a review perspective. Once this todo list reach 100%, I will assume it is finished and finalize review

We may even need to consider breaking this up if possible. This started out as an import clean up, and has morphed into a complete rewrite of our mappings, and thensome

[blitzmann]

Continue the discussion from #895...

I am reluctant about switching over to declarative, especially if the reason is simply something didn't import correctly. There's always a proper fix for that; changing the underlying database structure is not proper IMO. It's also not simply a different style; IIRC it has certain implications (that I've long forgotten - was a couple years ago I looked into switching it myself). Not only that, but we're not even using it correctly as it stands in that pull request from what I can see - I can't test as it crashes for me.

What were you working on when you were trying to do the import that was failing? Which commit was that?

[blitzmann]

Also finally getting around to reading the stuff here.

I have a backup of the code at the state where all the imports are fixed and Pyfa is functional again.

I would much prefer this PR being solely of imports. After evaluating them and making sure they work, any additional changes can be evaluated.

A follow up PR should be done removing the masked imports through types.

I agree, however it seems that was lumped into this one.

For example we could do from eos.db.saveddata.fit import Fit as eds_Fit

I agree we need unique class names. I frankly don't care if it's through aliasing or if the class themselves are renamed, though I prefer for the classes themselves to be renamed. So, (and I think I've said this somewhere), for service classes, FitService(), or CharacterService() would be ideal. eos Fit() should remain the same since that's our actual fit object that we need. I don't know where else Fit() is, but regardless we should be making a mapping document that documents old class / import to new convention so that everyone is on the same page. This should also go into another PR.

Eliminate locally scoped imports.

I am in support generally. I am concerned that you are taking generalized "don't do this" recommendations (like pep8) and following them without question. There are legitimate reasons to use locally scoped imports (just like there's legitimate reasons to use globals). Granted, I don't think pyfa specifically has any hard requirements for these things, and following standards are a good thing, I'm just making the case that there is not point in refactoring large swaths of code to eliminate a single scoped import. There are other fish to fry.

[Ebag333]

So this thing quickly got out of hand. sigh

Okay, here's what I'd like to do. Go all the way back to d963327 (or perhaps when it was merged) and get that cleaned up. Get Travis, Tox, and the pep8 stuff it complains about fixed, so that we're good there.

We might need to pull some changes after that (like renaming service if Travis keeps flipping out about it) but it should be fairly minimal changes, Pyfa was working at that point so it'd just be a matter of getting Travis and Tox happy.

That will largely refactor services. A lot of the changes you have issues with (like the DB table structure) aren't in play, and Eos is still a mess as far as imports go (relatively untouched).

For Eos, that's going to take some really careful refactoring because it is super ugly. I think it can be done, but it's going to be easier to do by doing it one little bit at a time rather than shotgun blasting it. (Sorry @a-tal , I know you wanted to fix all the problems at once, but I just don't think it's possible.)

We can leave the @a-tal branch open so we can reference it, there's a lot of good information about how we can refactor the various imports there.

I feel like the @a-tal branch isn't actually that far away from being functional again, but I don't know enough about sqlAlchemy and getting the environment setup to get it going again. Too much stuff got ripped out, so it'd take a lot of building up from scratch (not necessarily a bad thing, just beyond my ken).

[Ebag333]

See #907

[blitzmann]

Closing this as it will be broken into smaller PRs. :)