Skip to content
This repository has been archived by the owner on Nov 12, 2019. It is now read-only.

Fix deep object acess #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix deep object acess #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d656473
Minor fixes and changes
Thomas-P Feb 4, 2018
c80b8f3
add forgotten file
Thomas-P Feb 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,5 @@ typings/
# dotenv environment variables file
.env

# Intellij
.idea
2 changes: 1 addition & 1 deletion dist/src/message-builder.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@
*
*/
import { Message } from './message-constants';
export declare function getMessage(msg: Message, isAck: boolean): Buffer;
export declare function getMessage(message: Message, isAck: boolean): Buffer;
8 changes: 4 additions & 4 deletions dist/src/message-builder.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/src/message-builder.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/test/message-parserSpec.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/test/message-parserSpec.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/test/messagesSpec.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/test/messagesSpec.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions src/message-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,19 @@ import {
hasPayload,
} from './message-validator'

export function getMessage (msg: Message, isAck: boolean): Buffer {
const message = msg as any
export function getMessage (message: Message, isAck: boolean): Buffer {
let action = message.action

// convert action to write ack if necessary
if (message.isWriteAck && !isWriteAck(message.action as RECORD_ACTIONS)) {
action = ACTION_TO_WRITE_ACK[message.action]
}

const actionTopic = ACTIONS[message.topic]

if (message.isAck || isAck) {
action |= 0x80
if ((ACTIONS as any)[message.topic][message.action] === undefined) {
if (!actionTopic || actionTopic[message.action] === undefined) {
throw new Error(`message ${TOPIC[message.topic]} ${message.action} should not have an ack`)
}
}
Expand All @@ -84,7 +85,7 @@ export function getMessage (msg: Message, isAck: boolean): Buffer {

const metaError = validateMeta(message.topic, action, meta)
if (metaError) {
throw new Error(`invalid ${TOPIC[message.topic]} ${(ACTIONS as any)[message.topic][action] || action}: ${metaError}`)
throw new Error(`invalid ${TOPIC[message.topic]} ${actionTopic[action] || action}: ${metaError}`)
}

const metaStr = JSON.stringify(meta)
Expand Down
2 changes: 1 addition & 1 deletion src/message-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export interface RecordAckMessage extends RecordMessage {
}

export interface ParseError {
parseError: boolean
parseError: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this breaks in node 6 sadly

Copy link
Author

Choose a reason for hiding this comment

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

I will review this on the next weekend.

action: PARSER_ACTIONS

parsedMessage: Message
Expand Down
2 changes: 1 addition & 1 deletion test/message-parserSpec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('message parser', () => {
}
it (`parses ${TOPIC[topic]} messages ${authAction} correctly`, () => {
const result = parse(spec.urp.value)[0]
expect(result.parseError).to.be.undefined
expect(result.parseError).to.be.an('undefined')
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the difference with these out of curiosity?

Copy link
Author

Choose a reason for hiding this comment

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

This one

expect(result.parseError).to.be.undefined

hides information of an method call in an getter method called when you call an attribute. This might be fine, but on a reviewer perspective I'm not able to predict side effects or able to jump in the method, which is called by accessing the attribute. I'm also not so happy with my one, because it mixes types, which is also a problem. This one was provided as a solution on the documentation of chai in bdd style.

On my opinion the best is to switch to the assert methods which are also provided by chai. The isUndefined-method should do what expected.

if (!result.parseError &&
(!result.payloadEncoding || result.payloadEncoding === PAYLOAD_ENCODING.JSON)
) {
Expand Down
Loading