Enhancements to multigraph that I'd like to give back into the community - How?

CDuffy · 17 December 2017 09:48

I’ve benefited from all the hard work of everyone on this community and the tools themselves, hence want to give back some enhancements that I’ve been making.
I’ve been working on adding some extra capabilities to the control of multigraphs around allowing the user to select whether you see bars or lines and control the bar width for each series.
I’ve then tried to deal with defaults, to build upon existing saved multigraphs so that if nothing has been set and based some of this around what was already setup (e.g. using the datatype). I’ve also changed the mutligraph edit so that it only starts with the current level (basic) of settings and a radio to choose an advanced level.
For a stacked graph, the tooltip showed the height of the entry within the bar (i.e. including anything underneath it), hence have changed this to show just the data series height
An example of how this looks:

.
The example data is a little peculiar as I don’t have access to the data that I need, but will raise a separate topic to discuss this.
I realise that this should go in another part of the community, but have also changed mysolardivert app and tweaked the tooltip to show the unit as kWh for the Balance figure and then the show figures as kW when greater than 1,000 and removed the decimal places for the Watt measures. As below:

How can I feed this back as a candidate for you to hopefully then accept? I’m guessing this is via github, but as I’ve never used this before am unclear on how to use this and then how to propose my changes for your acceptance. Is there a short description already written that you can point me towards?
If you have conventions and approaches that I’m not following, then please let me know and I can tweak as appropriate.

glyn.hudson · 17 December 2017 22:01

Hi @CDuffy, welcome to the community .

It would be great to get some of your enhancements incorporated into Emoncms. The multigraph is part of Emoncms core, therefore you will need to submit a GitHub “pull request” to the emoncms core repository:

To do this you will first need to “fork” the repository to your own personal github account by clicking the fork button on github.com. The clone your fork, make your changes then push the changes to your fork. Once your changes on up on gtihub.com on your forked branch you can then submit a “pull request” to the emoncms repo.

See this guide explains the process well:

Github is really great, but can be a little bit confusing at first. It’s worth persisting since it’s such an unbelievably useful and powerful tool. We’re happy to help as much as we can.

CDuffy · 18 December 2017 11:22

Hi @glyn.hudson,

Thanks for the pointers, I’ve been following that guide and am obviously missing something as the push is complaining but seems to be stuck against the master branch and not my new one! I’m sure that I’ve made a simple newbie mistake!

To summarise my steps:

I created a fork in Github (GitHub - CDuffers/emoncms: Web-app for processing, logging and visualising energy, temperature and other environmental data)
I tried cloning from /var/www but as this said it already existed, I presumed this was sufficient to carry on
cd /var/www/emoncms/Modules/vis/visualisations
git branch multigraph_mysolardivert_enhancements
git checkout multigraph_mysolardivert_enhancements
git add multigraph/multigraph.js
git add ../Views/multigraph_edit.js
git commit - I received a read only filesystem error, hence ran an rpi-rw
git config to setup my userid and email
git status didn’t provide the suggested “Your branch is ahead …”
git push --set-upstream origin multigraph_mysolardivert_enhancements

Current status and git push output:

pi@emonpi(rw):visualisations$ git branch
  dev-mosquitto-php
  master
* multigraph_mysolardivert_enhancements
  stable
  symlinked_modules
pi@emonpi(rw):visualisations$ git status
On branch multigraph_mysolardivert_enhancements
nothing to commit, working directory clean
pi@emonpi(rw):visualisations$ git push --set-upstream origin multigraph_mysolardivert_enhancements
Username for 'https://github.com': cduffers
Password for 'https://[email protected]':
remote: Permission to emoncms/emoncms.git denied to CDuffers.
fatal: unable to access 'https://github.com/emoncms/emoncms.git/': The requested URL returned error: 403
pi@emonpi(rw):visualisations$

I’m guessing that the fact that it is referencing emoncms/emoncms.git rather than my branch is the real problem, I’m unsure what I’ve done/ not done to get stuck here!

Looking at my previous emoncms update log I was in the Stable branch, does this have any impact?

What can you suggest to push me forward please?

glyn.hudson · 18 December 2017 12:34

You will need to clone into an empty directory. I recomend you backup your local changes before cloning e.g.

rpi-rw
mv /var/www/emoncms /var/ww/emoncms-backup

then clone your fork

git clone https://github.com/CDuffers/emoncms.git

You should now have a new emoncms folder in /var/www, this folder will be under git version control you can check its status by entering the folder then running git status:

cd /var/www/emoncms
git status

Now you can re apply your changes, if all your changes are in multigraph.js you could just copy this file from your backup into the fresh emoncms folder:

`cp /var/www/emoncms-backup/Modules/vis/visualisations/multigraph/multigraph.js /var/www/emoncms/Modules/vis/visualisations/multigraph/multigraph.js

No need to git add the multigraph.js file since this file is already under version control. Git add is only required when adding a new file into the repo.

You can check that new changes are prent by running git status then git diff to show the local changes:

git status
git diff

Once you are happy with the changes you have made you can commit them

git commit -am "short description of the chnages you made

again I would recomend running git status to check all is well. Then when your ready you can push the changes to your remote fork. You can push multiple commits in one go. In fact it’s a good practive if your doing multiple changes to do multiple commits.

git push origin master

This will push the master branch to origin which is the default location for github.com. You can check the remotes which you have setup by running

git remote -v

which should return:

origin [email protected]:CDuffers/emoncms.git (fetch)
origin [email protected]:CDuffers/emoncms.git (push)

The steps I have listed above just commit you changes to the mater branch, this is fine if your just doing one set of changes. However it is good practice to create a new branch and commit your changes to this new branch since then you always have the option of easily reverting to the default master branch while testing your changes.

CDuffy · 18 December 2017 13:19

Thanks Glyn,
I ran the clone, then had to copy over the settings.php and then noticed a lot of the other aspects are also missing, I guess this is because of the modules etc that wouldn’t have been cloned.
Is it as simple as cloning those others repo, or is there anything else to include?
Having seen another thread about docker, I’m thinking I should be using that to tinker with rather than my emonhub!

CDuffy · 18 December 2017 22:48

I’ve read more around GitHub and am improving my understanding.

With this in mind and with my original copy of emoncms, I changed my Git URL because I’d seen the permission error

pi@emonpi(rw):.git$ git remote -v
origin  https://github.com/emoncms/emoncms.git (fetch)
origin  https://github.com/emoncms/emoncms.git (push)
pi@emonpi(rw):.git$ git remote set-url origin https://github.com/CDuffers/emoncms.git
pi@emonpi(rw):.git$ git remote -v
origin  https://github.com/CDuffers/emoncms.git (fetch)
origin  https://github.com/CDuffers/emoncms.git (push)
pi@emonpi(rw):multigraph$ git push --set-upstream origin multigraph_mysolardivert_enhancements
Username for 'https://github.com': cduffers
Password for 'https://[email protected]':
Counting objects: 9, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (9/9), 3.16 KiB | 0 bytes/s, done.
Total 9 (delta 7), reused 0 (delta 0)
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
To https://github.com/CDuffers/emoncms.git
 * [new branch]      multigraph_mysolardivert_enhancements -> multigraph_mysolardivert_enhancements
Branch multigraph_mysolardivert_enhancements set up to track remote branch multigraph_mysolardivert_enhancements from origin.
pi@emonpi(rw):multigraph$

I’ve subsequently followed the rest of the tutorial that you’d shared and have placed a pull request for the two files I’ve updated.

I’ll keep an eye out for responses and updates to this.

Thanks for your help and guidance

glyn.hudson · 19 December 2017 16:09

Nice work, congratulations!

Ah, I see you were trying to push via SSH without setting up your SSH key with github. This is what gave the permission error. Yes, changing the URL to HTTPS will allow you to push while entering your username and password each time. It’s a good idea to setup SSH keys in the future to allow you to connect to github.com via SSH which avoids having to enter your password each time:

Got your PR, thanks

I will wait for @TrystanLea to review, he is the main Emoncms developer. In the meantime there are a few minor issues that Codacy has thrown up. Could you take a look and correct if necessary. It’s possible to push additional commits to this PR by just pushing additional comits to your branch to make changes. No need to close then re-open the PR

https://www.codacy.com/app/pb66/emoncms/pullRequest?prid=1171676

CDuffy · 19 December 2017 19:40

Thanks Glyn, I’ll switch that over to ssh then.

I did see the Codacy complaints on my updates and then a broader set for the rest of the file. I’m happy to have a tweak at cleaning them up, but it had around 50 in total.

I’ve seen a load of Camel Case warnings (I had to read the notes to determine what the meant!), what’s your collective thoughts on adhering to them? Or just ignore?

I’ve signed in to Codacy via my GitHub account and given it access to my 2 forks. What is the best means to re-run the Codacy reviews? Make the changes locally, commit them, push them, to then check Codacy?

Apologies for all the newbie questions.

Paul · 19 December 2017 19:50

The general guide we’ve used is wherever possible to improve the Codacy score, however sometimes it’s not feasible or practical, especially when some of the errors occur due to historical coding formats & decisions elsewhere in the structure.
So yes, camelCase is preferable.

As you’ve already submitted the PR, if you commit further changes to your fork, Codacy should again run automatically, because they will form part of your PR.

Paul

glyn.hudson · 20 December 2017 15:54

Hi @CDuffy,

I’ve just tested the new bar graph feature. It seems to work well, one little issue I noticed is the drop down menu selects ‘bar’ as default when the default view is ‘line’. Then means in order to change the display to ‘bar’ the user needs to select line than back to bar to enable the bar view. See screecast illustrating the issue

Would this be possible to fix?

Another idea I had would be to make the multigraph interface cleaner would be to wrap up all the extra functions such as ‘stac’, lock axis, show-tag-name etc in the advanced section.

CDuffy · 20 December 2017 17:16

Hi @glyn.hudson,

I’m always up for constructive feedback.

Yep, that’s a good idea. I’ve moved the bottom options and also skip missing/ stacked into “advanced” as follows:

The defaulting to bars was something that I’d noticed and fixed about the time you’d sent this message. And was due to my updates from Codacy suggesting a === and me assuming that ‘datatype’ was stored as a numeric. I’ve converted the comparison to a string to fix this.

I had wondered if we remove the bars option for datatype=1 which I believe covers the ‘power’ style data sources and potentially the “lines with steps” as well, which could mean we just remove that drop down entirely. Otherwise I can think about how to revise the barwidth to support this datatype - What do you think?

As you’ll have seen with the number of additional commits, I somewhat daftly decided to learn more about these coding best practices by following the Codacy advice and have regretted it a little!. The knock on is that I’ve removed a large number of minor misdemeanors, but am left with a lot of Generic Object Injection Sink (security/detect-object-injection), the guidance isn’t clear to me, but believe that there is a concern about code injection possibilities, but am otherwise unsure on what we could actually do here!?

Clive.

glyn.hudson · 20 December 2017 22:15

Awesome, I’ve just tested. That looks great

Great, yup I can confirm it’s working well for me now.

Yes, that sounds like a good idea. Unless you can think of a fridge case when it might be useful to have bar graph for data type 1. I don’t see any particular harm in leaving the option available as long as it’s not selected by default. I’ll leave the decision up to you. I’m happy to merge the PR as it is currently. Let me know when you are ready.

Would anyone else like to test the PR in question?:

As @Paul said above Codacy is only a guide. There should not be any concert about injection with your changes.

Paul · 20 December 2017 22:44

I can’t test at the moment, as I’m travelling, but did try the changes a number of commits ago and it looked like a welcome addition (although I had the same thoughts about moving options to advanced - as Glyn posted earlier).

The code quality is looking much healthier now, and of course many of the issues were inherited, so thanks to Clive for his persistence!

Paul

glyn.hudson · 23 December 2017 00:30

PR has been merged. Thanks a lot @CDuffy