-
Notifications
You must be signed in to change notification settings - Fork 412
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
Feature/1463 insurance #1699
base: master
Are you sure you want to change the base?
Feature/1463 insurance #1699
Conversation
…hich has data preloaded
o/ Sorry, this dropped off my radar! So, first thing, thanks for attempting to tackle this! Including insurance prices has definitely been on the radar for a bit, and this seems to work in general. This is a strong first step, but lets make it even stronger! I'll start with a review current progress, and move into what I think still needs to be done
All that being said, I understand that there wasn't any previous discussion on where this data should go, so it's reasonable to think the stats view as it's easy to develop for. And there is the option of turning it off / on (but we also need to consider the fact that if it's turned off, there's currently no way of accessing that information at all - see below for discussion on this). I'm okay with keeping the view as it is, but with the caveat that it should be off by default, and have user's opt in if they have a need to see this information on a regular basis.
So that covers the current code progress. All in all, I see good initial set up here. Lets talk about next steps 😄
We can also save the last fetched date, and then come up with a common sense cache time (48 hours? how often does insurance price substantially change?). If the timer has passed when loading the Service, go ahead and load the old data, but then kick off an update as well
I think if we can get those additional things implemented and tweaked, we would be in good shape to merge this in for the next release! 🎉 Also, another question: What happens if the insurance service is called upon for prices, but the api fetch hasn't completed yet? If we haven't tested this, then it's something to think about. Also need to consider not being able to access the API for some reason (firewall, no internet, etc) and make sure things don't break. |
Excellent, thanks for taking a look at this. I've moved to a different state so I've haven't had time to look at Pyfa in a while, but I'm back and mostly setup in the new pad. To point 1.1, I agree, showing all prices is super clunky, I think In the application I have I only display platinum, almost everyone I asked either gets plat or no insurance. It does seem like a huge amount of information and they probably only want the plat. I do like the idea of making it opt in, because most people probably won't even want this. I believe insurance is calculated daily at downtime, basing off that days mineral prices. (Not 100% sure though, can't find a source) I'll probably end up running some tests hitting the ESI endpoint every 10 minutes and just checking when the values change. I'll look into all the other things you've mentioned here when I work on this next, thanks for the thought out response/analysis. |
#1463 I used priceViewFull as example so that this view is doing things as consistently as possible. I'm not sure if pyfa.py is the right place to load insurance data, and would be open to suggestions on where to put it. Another note, the get function in ESI access was modified so that the Insurance could use it, but perhaps it needs its own function for clarity/readability. And one more note, context menu options were added for column display, but it requires restart with the way populating the stats panes work.