Skip to content

shiritori: update dependencies #17

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 17, 2019
Merged

shiritori: update dependencies #17

merged 8 commits into from
Apr 17, 2019

Conversation

proppy
Copy link
Contributor

@proppy proppy commented Apr 10, 2019 •
edited
Loading

  • bump all dependencies version
  • add missing fields to package.json
  • adapt to kuroshiro breaking change
  • switch to Promise for dispatching result

Test report:

npm test

  ✔ check: loose used (1.3s)
  ✔ interact: lose (1.3s)
  ✔ check: loose ん (1.3s)
  ✔ interact: next (1.3s)
  ✔ check: continue (1.3s)
  ✔ check: loose chain (1.3s)
  ✔ check: kanji (1.3s)
  ✔ interact: win used (1.3s)
  ✔ interact: win ん (1.3s)
  ✔ interact: error (1.3s)
  ✔ check: ぁぃぅぇぉゃゅょ rules (1.3s)
  ✔ kanas (1.3s)
  ✔ check: ー rules (1.3s)

  13 tests passed

c8 coverage report (not 100% because of bcoe/c8#72)

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    92.06 |    72.55 |      100 |    92.06 |                   |
 index.js |    92.06 |    72.55 |      100 |    92.06 |... 97,110,118,124 |
----------|----------|----------|----------|----------|-------------------|

nyc coveratge report:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |      100 |      100 |      100 |      100 |                   |
 index.js |      100 |      100 |      100 |      100 |                   |
----------|----------|----------|----------|----------|-------------------|

/cc @yoichiro @atulep @Fleker @smishra2

@proppy
shiritori: update dependencies
b101664 
- bump all dependencies version
- add missing fields to package.json
- adapt to kuroshiro breaking change
- switch to Promise for dispatching result
@proppy proppy requested a review from Fleker April 10, 2019 08:47
Fleker
Fleker requested changes Apr 10, 2019
View reviewed changes
proppy added 4 commits April 11, 2019 16:56
@proppy
shiritori: use async/await
09e6bbf
@proppy
shiritori: update package-lock
bf90ac2
@proppy
shiritori: lint
601eaf7
@proppy
shiritori: Exception -> Enum
ca5bf69 
- return structured enum instead of exception
- refresh tests dependencies and lint rules
@proppy proppy requested a review from Fleker April 12, 2019 05:07
Fleker
Fleker requested changes Apr 12, 2019
View reviewed changes
functions/index.js
});
});
app.intent('actions.intent.TEXT', async (conv, input) => {
const result = await shiritori.interact(dict, input, conv.data.used);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to catch the Error anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the previous code was doing nothing but re-throwing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you at least catch the error here and state an error message? Otherwise the function will crash and so will your action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, actually removed the error handling, since it wasn't valid, see ce5269c commit message for more details.

proppy added 3 commits April 15, 2019 10:56
@proppy
shiritori: LOOSE -> LOSE
fd70aed
@proppy
shiritori: remove error
ce5269c 
The error was actually checking for an empty 'string' rather than a
empty list of words.

The former would not happen as long as the dictionary of transcription
is valid, and the later is already covered by former winning condition
(WIN_USED).
@proppy
shiritori/test: add coverage
4a0233f
@Fleker
Copy link
Member

Fleker commented Apr 16, 2019

LGTM

1 similar comment
@Fleker
Copy link
Member

Fleker commented Apr 16, 2019

LGTM

@proppy proppy merged commit 30b1a43 into actions-on-google:master Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fleker Fleker Fleker approved these changes

Assignees

@smishra2 smishra2

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@proppy @Fleker @smishra2 @googlebot