-
Notifications
You must be signed in to change notification settings - Fork 45
Conversation
2d60abd
to
ea95829
Compare
7b1d816
to
217ed6d
Compare
217ed6d
to
cc54ff7
Compare
- [Angular Hint DOM](https://github.com/angular/angular-hint-dom) | ||
- [Angular Hint Events](https://github.com/angular/angular-hint-events) | ||
- [Angular Hint Interpolation](https://github.com/angular/angular-hint-interpolation) | ||
- [Angular Hint Modules](https://github.com/angular/angular-hint-modules) |
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 guess all these repos have been consumed into this one?
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.
TBH, I have no idea what happened to them 😁
I know the repositories do not exist any more (although they used to) and src/modules has some files that seem to correspond to some (although not all) of the modules. So I assume some got merged, some got removed(?).
For AngularHint: | ||
|
||
```shell | ||
gulp |
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.
Do you no longer need to check in built files?
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 haven't changed anything in that regard. But this step is run after you have commited your changes so it is not about including built files in the commit. This step just seemed a duplicate of step 5 (running tests), with extra info about modules, which do not exist any more.
(Also note that the default gulp
task runs the tests.)
scripts/test_on_sauce.sh
Outdated
|
||
wait %2 | ||
# TODO(gkalpak): Fix e2e tests and re-enable. | ||
#yarn test-e2e --ignore-engines |
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.
:-)
This is a bit sneaky no?
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.
The tests don't work so disable them?
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.
While refactoring the e2e tests and getting them to at least run (and fail), I found out they seem like a WIP (in its failry early stages 😃). There are very few tests, several broken tests and a large number of unused files (that I assume were supposed to be turned into meaningful tests).
Also note that:
- The line that was supposed to run e2e tests was commented out before (so my PR doesn't change that).
- The tests were already broken (my PR doesn't break any tests).
- I can be sneaky, but I usually choose not to 😛
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.
From where is dist/hint.js
built?
Slightly concerned about the disabling of the e2e tests.
Other than that it all LGTM
It is built by running browserifying hint.js (using this gulp task).
As explained here, this PR doesn't change anything in that regard.
I'll take that 😃 |
@gkalpak did you get write access then? |
I did :) |
Nothing new. Mainly updating dependencies, fixing scripts and configs, switching to yarn.