[Console] Token Iterator #46793

jloleysens · 2019-09-27T14:37:22Z

Summary

More details in phase 2.1 of this roadmap item: https://github.com/elastic/elasticsearch-team/issues/217

Before ready for review

Get CI working again
Finish TODOs
More manual Testing

elasticmachine · 2019-09-27T14:37:24Z

Pinging @elastic/es-ui

elasticmachine · 2019-09-27T15:54:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

This looks great! I was able to follow the code pretty easily but had a couple questions and some really minor suggestions -- feel free to merge without implementing them if you prefer.

src/legacy/core_plugins/console/np_ready/public/interfaces/editor.ts

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.test.ts

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.ts

cjcenizal · 2019-09-28T00:24:31Z

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.ts

+  let acc = '';
+  return tokens.map(token => {
+    const column = acc.length + 1;
+    acc += token.value || '';


When is token.value falsey? It seems like we would get into a strange situation if this occurred, since appending an empty string won't change the length of acc and yet we're using the length of acc to specify column on line 30. So we'd have multiple tokens occupying the same column.

If token.value can never be falsey then maybe we should just remove this default value? Or, if it can be falsey, then I wonder if this represents an error state -- in which case maybe we should throw? Or if it's not an error state, should we just skip the token without mutating acc and without creating a token, and move on to the next one?

Great catch! You are totally correct that this should be an error case. In fact, most tokenizers I've used will error out if you match an empty string or if it doesn't have a way to tokenize a string. Will remove the default.

👍 Maybe some unit tests for this function or testing this error case through its consumers would be good too.

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.ts

src/legacy/core_plugins/console/np_ready/public/lib/token_iterator/token_iterator.ts

src/legacy/core_plugins/console/np_ready/public/lib/token_iterator/token_iterator.test.ts

src/legacy/core_plugins/console/np_ready/public/lib/token_iterator/token_iterator.ts

A lot of things broke!

elasticmachine · 2019-10-01T16:47:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-03T16:43:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4e1f9e3

Update token iterator and token provider behviour to go to the first token BEHIND the cursor Update test coverage Get legacy tests working with new token iterator

elasticmachine · 2019-10-04T17:33:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 6fbbe7f

Cleaned up interface arguments for existing insert method too.

jloleysens · 2019-10-07T10:30:40Z

@elasticmachine merge upstream

… into console-token-iterator * 'console-token-iterator' of github.com:jloleysens/kibana: (184 commits) [functional/services] update webdriver lib and types (elastic#47381) Standardizing IconField implementation across the app (elastic#47196) Move ui/value_suggestions ⇒ NP data plugin (elastic#45762) Remove ui/persisted_log - Part 2 (elastic#47236) Update gulp related packages (elastic#47421) Update dependency idx to ^2.5.6 (elastic#47399) try running fewer jobs in parallel on the same worker (elastic#47403) Update webpack related packages (elastic#47402) Update jsonwebtoken related packages (elastic#47400) Update gulp related packages (major) (elastic#46665) Update dependency prettier to ^1.18.2 (elastic#47340) Update dependency @types/puppeteer to ^1.20.1 (elastic#47339) Update dependency @elastic/elasticsearch to ^7.4.0 (elastic#47338) Update dependency tar-fs to ^1.16.3 (elastic#47341) [Code] Code Integrator Component (elastic#47180) [Canvas][i18n] Sidebar (elastic#46090) Generate uuid in task Manager as Kibana uuid may not yet have been initialised [Code] Embedded Code Snippet Component (elastic#47183) Revert "Add pipeline for flaky test runner job (elastic#46740)" SearchSource: fix docvalue_fields and fields intersection logic (elastic#46724) ...

elasticmachine · 2019-10-07T11:36:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: df4ca33

jloleysens · 2019-10-14T08:40:59Z

@elasticmachine merge upstream

elasticmachine · 2019-10-14T09:49:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7f9be47

jloleysens · 2019-10-14T13:04:55Z

Retest

elasticmachine · 2019-10-14T14:10:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7f9be47

sebelga

Awseome work @jloleysens ! I made a few comments that you can feel free to address or not. Most of them about some preferences.

There seems to be a regression though when I tested locally, trying to "Open documentation" on a request throws an error in the console and does not open the docs.

Once this is fixed I'll approve. thanks!

src/legacy/core_plugins/console/np_ready/public/application/models/legacy_editor.ts

sebelga · 2019-10-14T13:44:37Z

src/legacy/core_plugins/console/np_ready/public/application/factories/token_iterator.ts

@@ -0,0 +1,30 @@
+/*


I personally prefer to group files by "feature" than by "type". So, in this case, have everything under a "token_iterator" folder. It's just a preference though.

src/legacy/core_plugins/console/np_ready/public/lib/token_iterator/token_iterator.ts

src/legacy/core_plugins/console/np_ready/public/interfaces/tokens_provider.ts

src/legacy/core_plugins/console/np_ready/public/lib/token_iterator/token_iterator.ts

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.test.ts

src/legacy/core_plugins/console/public/quarantined/src/input.ts

sebelga · 2019-10-14T15:16:56Z

src/legacy/core_plugins/console/np_ready/public/interfaces/token_iterator.ts

+  /**
+   * Report the token under the iterator's cursor.
+   */
+  getCurrentToken(): Token | null;


Should we use class getters for those?

(in response to your suggestion about getters above too)

This interface is designed to be an in-place replacement for https://github.com/ajaxorg/ace/blob/master/lib/ace/token_iterator.js.

This reduces the surface area for changes for now. But otherwise I agree, getters would be a nice addition/change!

Got it, thanks for explaining

src/legacy/core_plugins/console/np_ready/public/lib/ace_token_provider/token_provider.ts

elasticmachine · 2019-10-15T10:52:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d08e14b

sebelga

LGTM. Tested locally and the documentation link now works as expected. Thanks!

jloleysens · 2019-10-22T08:28:36Z

@sebelga Great, thanks! I'll fix the tests and merge!

jloleysens · 2019-10-22T08:32:54Z

@elasticmachine merge upstream

elasticmachine · 2019-10-22T09:43:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1e58970

elasticmachine · 2019-10-22T11:02:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 23bf85f

jloleysens · 2019-10-22T13:09:46Z

@elasticmachine merge upstream

elasticmachine · 2019-10-22T14:28:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c052cf6

* Start of token provider and token iterator * Update Range interface * Implement early feedback * getTokenAt tests and update * First pass! Autocomplete.js -> Autocomplete.ts A lot of things broke! * Added factories and models * Lots of WiP * Initial wiring up of new token iterator * Fix types * Minor updates to token iterator * Fixed legacy editor.test.js tests (they were never running properly) Update token iterator and token provider behviour to go to the first token BEHIND the cursor Update test coverage Get legacy tests working with new token iterator * Address documentation Todos and implement missing `insert` method. Cleaned up interface arguments for existing insert method too. * Document range and line mode * Fix broken async test in token provider (done callback) * Fix path * Slight update to token_provider and token_iterator to make code more readable * Implement PR feedback (including fix for open documentation) * Remove token_iterator interface * initializeInput -> initializeEditor for quarantined integration tests * Update input init mock

Start of token provider and token iterator

3a0cb59

jloleysens added Feature:Console Dev Tools Console Feature enhancement New value added to drive a business result refactoring Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 labels Sep 27, 2019

jloleysens requested a review from cjcenizal September 27, 2019 14:37

cjcenizal approved these changes Sep 28, 2019

View reviewed changes

jloleysens added 7 commits September 30, 2019 12:29

Update Range interface

1ef3880

Implement early feedback

1ce3971

getTokenAt tests and update

8267498

First pass! Autocomplete.js -> Autocomplete.ts

0d9a211

A lot of things broke!

Added factories and models

40e4076

Lots of WiP

7a97f53

Initial wiring up of new token iterator

fa55647

Fix types

4e1f9e3

jloleysens added 2 commits October 4, 2019 13:07

Minor updates to token iterator

dee0fda

Fixed legacy editor.test.js tests (they were never running properly)

6fbbe7f

Update token iterator and token provider behviour to go to the first token BEHIND the cursor Update test coverage Get legacy tests working with new token iterator

jloleysens requested a review from sebelga October 4, 2019 16:17

Address documentation Todos and implement missing insert method.

5152927

Cleaned up interface arguments for existing insert method too.

elasticmachine and others added 3 commits October 7, 2019 03:30

Merge branch 'master' into console-token-iterator

df4ca33

Document range and line mode

d2ff6a2

Merge branch 'master' into console-token-iterator

7f9be47

sebelga requested changes Oct 14, 2019

View reviewed changes

jloleysens added 2 commits October 15, 2019 11:28

Implement PR feedback (including fix for open documentation)

de938a3

Remove token_iterator interface

d08e14b

sebelga self-requested a review October 22, 2019 07:21

sebelga approved these changes Oct 22, 2019

View reviewed changes

initializeInput -> initializeEditor for quarantined integration tests

aa0c886

Merge branch 'master' into console-token-iterator

1e58970

Update input init mock

23bf85f

Merge branch 'master' into console-token-iterator

c052cf6

jloleysens merged commit 86b72d3 into elastic:master Oct 22, 2019

jloleysens deleted the console-token-iterator branch October 22, 2019 14:31

jloleysens added the v8.0.0 label Oct 25, 2019

This was referenced Oct 25, 2019

[Console][Legacy Editor] Ace Range implementation not being used #49352

Merged

[Console] Restore completer behaviour #49422

Merged

[7.x] [Console] Token Iterator (#46793) #49463

Merged

jloleysens mentioned this pull request Oct 28, 2019

[Dev Tools] Autocomplete not functioning in master #49365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Token Iterator #46793

[Console] Token Iterator #46793

jloleysens commented Sep 27, 2019 •

edited

elasticmachine commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

cjcenizal left a comment

cjcenizal Sep 28, 2019

jloleysens Sep 30, 2019

cjcenizal Sep 30, 2019

elasticmachine commented Oct 1, 2019

elasticmachine commented Oct 3, 2019

elasticmachine commented Oct 4, 2019

jloleysens commented Oct 7, 2019

elasticmachine commented Oct 7, 2019

jloleysens commented Oct 14, 2019

elasticmachine commented Oct 14, 2019

jloleysens commented Oct 14, 2019

elasticmachine commented Oct 14, 2019

sebelga left a comment

sebelga Oct 14, 2019

sebelga Oct 14, 2019

jloleysens Oct 15, 2019

sebelga Oct 16, 2019

elasticmachine commented Oct 15, 2019

sebelga left a comment

jloleysens commented Oct 22, 2019

jloleysens commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

jloleysens commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

[Console] Token Iterator #46793

[Console] Token Iterator #46793

Conversation

jloleysens commented Sep 27, 2019 • edited

Summary

Before ready for review

elasticmachine commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

💚 Build Succeeded

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Sep 28, 2019

Choose a reason for hiding this comment

jloleysens Sep 30, 2019

Choose a reason for hiding this comment

cjcenizal Sep 30, 2019

Choose a reason for hiding this comment

elasticmachine commented Oct 1, 2019

💔 Build Failed

elasticmachine commented Oct 3, 2019

💔 Build Failed

elasticmachine commented Oct 4, 2019

💔 Build Failed

jloleysens commented Oct 7, 2019

elasticmachine commented Oct 7, 2019

💔 Build Failed

jloleysens commented Oct 14, 2019

elasticmachine commented Oct 14, 2019

💔 Build Failed

jloleysens commented Oct 14, 2019

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

sebelga left a comment

Choose a reason for hiding this comment

sebelga Oct 14, 2019

Choose a reason for hiding this comment

sebelga Oct 14, 2019

Choose a reason for hiding this comment

jloleysens Oct 15, 2019

Choose a reason for hiding this comment

sebelga Oct 16, 2019

Choose a reason for hiding this comment

elasticmachine commented Oct 15, 2019

💔 Build Failed

sebelga left a comment

Choose a reason for hiding this comment

jloleysens commented Oct 22, 2019

jloleysens commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

💔 Build Failed

elasticmachine commented Oct 22, 2019

💔 Build Failed

jloleysens commented Oct 22, 2019

elasticmachine commented Oct 22, 2019

💚 Build Succeeded

jloleysens commented Sep 27, 2019 •

edited