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

Modular app structure and clean up #53

Merged
merged 10 commits into from Jul 16, 2018

Conversation

Projects
None yet
3 participants
@adminde
Copy link
Contributor

adminde commented Jul 3, 2018

Hi guys,

as discussed in #48, I just started cleaning up the app module to be more consistend with emoncms naming and structure conventions.
Main change is the addition of a modular structure in Modules/app/Apps to support e.g. softlinking of custom apps to the directory.

--app
  |--Apps
  |  |--OpenEnergyMonitor
  |  |  |--myelectric
  |  |    |--app.json
  |  |    |--myelectric.php
  |  |  |--mysolarpv
  |  |    |--app.json
  |  |    |--mysolarpv.php

A new app_settings.php file may be optionally created to whitelist apps by their name and change the app/Apps/ dir.
As a bonus, I noticed that newly created apps were not opened directly in config mode, hence I added that.

All credits to @pb66 and @greeebs for the original idea and first implementation of a modular structure.
Most changes were Renames so I'm pretty confident this will work without any unexpected bugs ;)

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Jul 3, 2018

I suggest you add some error checking for poorly formed .json files (with a missing title or description primarily) - its a straightforward check that I added to my fork last night... it should go around line 46 of the new app_model.php...

This is much neater than my shoe-horn approach of minimal changes :) I was too intimidated by the controller code :-o

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 4, 2018

Thanks @adminde

Testing here I have come across the following error, I think it might be a simple case issue?

Fatal error: Uncaught exception 'UnexpectedValueException' with message 'RecursiveDirectoryIterator::__construct(Modules/app/Apps/): failed to open dir: No such file or directory' in /var/www/emoncms/Modules/app/app_model.php:37 Stack trace: #0 /var/www/emoncms/Modules/app/app_model.php(37): RecursiveDirectoryIterator->__construct('Modules/app/App...') #1 /var/www/emoncms/Modules/app/app_controller.php(24): AppConfig->get_available() #2 /var/www/emoncms/core.php(64): app_controller() #3 /var/www/emoncms/index.php(189): controller('app') #4 {main} thrown in /var/www/emoncms/Modules/app/app_model.php on line 37

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 4, 2018

Fixing line 37 then results in another case related error:

Warning: include(Modules/app/Apps/blank/blank.php): failed to open stream: No such file or directory in /var/www/emoncms/core.php on line 75

Warning: include(): Failed opening 'Modules/app/Apps/blank/blank.php' for inclusion (include_path='.:/usr/share/php:/usr/share/pear') in /var/www/emoncms/core.php on line 75

@adminde

This comment has been minimized.

Copy link
Contributor

adminde commented Jul 5, 2018

Well that was incredibly stupid 😉
I accidently used usort for the associative array of available apps, effectivly removing all app ids. Sorry about that

@adminde

This comment has been minimized.

Copy link
Contributor

adminde commented Jul 5, 2018

And I realised that renaming the Modules/app/apps dir to an uppercase Apps was incredibly stupid as well.
Now everything should work as indented. Sorry again.

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 11, 2018

Thanks @adminde sorry for the delay. Testing again here I get the error "ReferenceError: config is not defined" on line 274 of view:

config.app = {
    "use":
@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 11, 2018

Thankyou for your work on this @adminde and the device module as always :)

@adminde

This comment has been minimized.

Copy link
Contributor

adminde commented Jul 13, 2018

Well, that one was exactly the same problem as the last one... git does not automatically rename dirs for changed capitalization.
I changed that now for the Lib dir... very annoying... I felt so confident about these changes but without cloning the repo from scratch I would've never even noticed this^^

btw, @TrystanLea what would be your opinion regarding the consistent naming scheme for some of the directories as discussed in #48 ? Lib and Views always Uppercase as done in most other modules?

and as a big fan of this work, I'm always happy to contribute 😉

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 16, 2018

Great @adminde that all works! :)

@TrystanLea TrystanLea merged commit b0646a7 into emoncms:master Jul 16, 2018

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 16, 2018

btw, @TrystanLea what would be your opinion regarding the consistent naming scheme for some of the directories as discussed in #48 ? Lib and Views always Uppercase as done in most other modules?

I dont really have a strong opinion on this one, I should probably have used lower case throughout early on...

@glynhudson glynhudson referenced this pull request Oct 17, 2018

Merged

stable merge V1.2.0 #57

@adminde adminde deleted the isc-konstanz:modular branch Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment