Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Several changes and new features #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

nfsprodriver
Copy link

This pull request may not be finished yet, so @philipptrenz please add your comments or contribute (in a new branch would be best).

Overview of changes in this pull request:

  • added a total chart (based on year chart calculations)
  • fixed capitalization for displayed strings
  • improved labels of data
  • walk through days (with existent data!) by clicking on bars
  • unified calculations of sums (SQL sums and python sums are unequal!)
  • optimized loading times by only refreshing month and day data (we may think about that?)
  • updated README.md according to these changes (we may mention more of these)

When some changes are missing, I'll add these to the changelog.

@philipptrenz
Copy link
Owner

Hey @nfsprodriver, many thanks for your contribution!
Just tested it, it adds a lot of features I also wanted and it works great. Which additional features do you plan to implement?

@philipptrenz
Copy link
Owner

Moved your changes into nfsprodriver-dev branch

@philipptrenz
Copy link
Owner

As pylint failed, I took a look into util/database.py and I understand what you tried to achieve. Nevertheless, from my point of view the backend processing should be stateless, as there can be more than one client connected and the API should be REST-like.

So the frontend should specify the data it wants to update. This could be done within the JSON like so:

static/js/main.js, line 449:

var request_data = {
    date: currentDay
    requested_data: {
        day = true,
        month = true,
        year = false,
        years = false,
        total = false 
    }
};

Then the backend builds the response JSON only with the requested data.

OR

My preferred solution would be to have several API endpoints for the different data entities. So that the frontend no longer requests to /update, but instead to

GET /day
GET /month
GET /year
GET /total

For initial loading there could be a GET /all endpoint and afterwards only the necessary updates get requested (if needed with multiple queries).

@nfsprodriver
Copy link
Author

Thanks @philipptrenz for the fast answer. For now, I don't have any idea for more features, which are necessary in the PR, so I'd like to feature freeze it.
Talking about the update thing your point is very good to move it out of the backend. I don't know much about REST-API than it may need some work. So for this PR I'll try to implement your JSON proposal.
If there's anything else, which needs to be changed before merging, please tell me ;)

@nfsprodriver
Copy link
Author

Did you mean like this 8dca723 ?

@nfsprodriver
Copy link
Author

One little thing: In the total chart the first year is cut off a little bit. May we add the year before first data on X-axis or do you have a better idea?

@philipptrenz
Copy link
Owner

philipptrenz commented Aug 14, 2019

Found the following issues:

  • Pulled your latest changes and found, that your changes increases the loading time by a factor of 30. The current master branch loads within 130ms on my machine, with your changes it's around 4 seconds.
  • Your Python code is not PEP8 conform, you can check that by using a linting tool like pylint
  • Your global variables in util/database.py do not fit the object-oriented style of this class

One little thing: In the total chart the first year is cut off a little bit. May we add the year before first data on X-axis or do you have a better idea?

Yeah, this can be changed by manipulating the from and to values of the chart, like within the following lines:

// update scale
all.tot.interval.from = moment.unix(all.tot.interval.from).subtract('months', ???);
all.tot.interval.to = moment.unix(all.tot.interval.to).add('months', ???);
totChart.data.labels = [
    all.tot.interval.from,
    all.tot.interval.to
];

@nfsprodriver
Copy link
Author

About the conformidity, pylint does not only detects my lines as unconfirm. Can you explain, what you mean? I'm very new to this topic.

@nfsprodriver
Copy link
Author

This is a proposal, which you could have meant a6b54dd

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants