Skip to content

feat(PBOT-103): multi word label support #34

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

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

feat(PBOT-103): multi word label support #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f126ece
wip: update to AddLinkToProjectAction test
colbyallenc Jul 11, 2019
1c18c76
feat: /project add supports multi-word labels for links
colbyallenc Jul 11, 2019
08186b5
ref: execute returns AddLinkToProjevtResponse'
colbyallenc Jul 11, 2019
ba6c39e
wip: correction to how linkparams labels are stored
colbyallenc Jul 11, 2019
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
18 changes: 11 additions & 7 deletions lib/actions/addLinkToProjectAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,20 @@ class AddLinkToProjectAction {
parseParams(userInputString = '') {
assert(typeof userInputString === 'string');
const parsedUserInput = userInputString.split(' ');

let linkParams;
if (parsedUserInput.length < 2) {
throw new TypeError('invalid input');
}

const linkParams = {
label: parsedUserInput[1],
link: parsedUserInput[2],
};
if (parsedUserInput.length >= 4) {
linkParams = {
label: parsedUserInput.slice(1, 3).join(' '),
Copy link
Copy Markdown

@nrambeck nrambeck Jul 11, 2019

Choose a reason for hiding this comment

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

This method is on the right track, but will only work if 3 words are used for the label. If only 1 or 2 words are used for the label, then the slice() will end up including the URL. If 4+ words are used for the label, then only the first 3 will be included in the label.

This function would a great candidate for test-driven coding. Write your test cases first with multiple scenarios you would expect this to work with, then change your code to make the tests pass.

Copy link
Copy Markdown

@nrambeck nrambeck Jul 11, 2019

Choose a reason for hiding this comment

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

I'm also concerned that his approach to accepting multiple words will cause future problems. What happens when you add a new parameter that can also be multiple words? How will you be able to distinguish between where one parameter ends and the other one starts?

Maybe you could support this method at first, then look at another method (ie. multi-word params in quotes) in the future.

link: parsedUserInput.pop(),
};
} else {
linkParams = { label: parsedUserInput[1], link: parsedUserInput[2] };
}

return linkParams;
}

Expand All @@ -29,8 +34,7 @@ class AddLinkToProjectAction {
const projectLink = new this.projectLink(context.project, paramsObj.label, paramsObj.link);
await projectLink.save(context.driver);
// eslint-disable-next-line new-cap
const response = new AddLinkToProjectResponse(paramsObj.label, paramsObj.link);
return response;
return new AddLinkToProjectResponse(paramsObj.label, paramsObj.link);
}
}

Expand Down
1 change: 1 addition & 0 deletions test/addLinkToProjectAction.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('Add Link to Project Action', () => {
await knex.select('*')
.from('project_links')
.where({ project_id: 'a123' })
.andWhere({ url: 'toProjectExecute.com' })
.then(rows => expect(rows[0].label).to.equal('addlink'))
.then(knex.select('label')
.from('project_links')
Expand Down