-
Notifications
You must be signed in to change notification settings - Fork 9
Adding elm-analyse to Travis #167
base: master
Are you sure you want to change the base?
Conversation
|
Apparently the build fails now for the elm-analyse warnings: |
ci-scripts/install_elm_analyse.sh
Outdated
| fi | ||
|
|
||
| npm install -g [email protected] | ||
| npm install -g elm-analyse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let"s always specify a version, if we don't freeze it, we will end up with tons of failing Travis jobs on the day of a major release of Elm Analyse.
… package, it referred to non-existing functions and so on
| viewActiveIncidents items = | ||
| let | ||
| orderedIncidentes = | ||
| getOrderedIncidents items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bitamar As there's no getOrderedIncidents function in the whole project, I decided to do a git rm on the whole file, everything compiles cleanly without it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
| HandleFetchedItem itemId (Ok item) -> | ||
| let | ||
| -- Let Item settings fetch own data. | ||
| -- @todo: Pass the activePage here, so we can fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the rationale behind this todo.

Drupal Elm Starter does provide a pattern to selectively fetch items, It does fetch all items upon start, but we do it like this for many real-world apps. I'd not invest more in resolving/restoring this todo.
^^ @bitamar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's just remove it then.
|
@bitamar after pushing two more commits and did a check, in my opinion it's close to be ready, as it's your PR, i did not set it to ready to be reviewed however. :) I wanted to check how to add it to PHPStorm, to execute automatically, but I failed. |
|
Thanks! I just didn't have time to get to it until now. I'll try to run it in phpstorm, and update the readme. I also want to include the importAll/exposeAll tests, but these can also be a follow up. |
|
I couldn't find a way to elm-analyse a single file, and running it on all the project is a bit much for every save. So I think it should be something you do manually before committing elm files. Or do you have a better idea? |
|
@bitamar Let's stick to manual execution, in the issue queue, there are debates about IDE integration, but without a solution (stil4m/elm-analyse#77), so likely it's not a low-hanging fruit. |
#166