Repository navigation
Fix simple cipher - #536
Conversation
|
@matthewmorgan hi! not sure why travis is failing - looks like some of the specs failed. i changed some code on the spec file to correct lint errors - maybe i missed something there? |
matthewmorgan
left a comment
There was a problem hiding this comment.
@leahzxxz See my comment about the linting error etc in the specfile. This is a case where we need to purposely disable one rule for a limited set of tests.
| 'use strict'; | ||
|
|
||
| var ALPHABET = 'abcdefghijklmnopqrstuvwxyz'; | ||
| const ALPHABET = 'abcdefghijklmnopqrstuvwxyz'; |
There was a problem hiding this comment.
As I mentioned in the previous PR, we want to always use var on this track.
There was a problem hiding this comment.
Yep will change - thanks!
| expect( function () { | ||
| new Cipher('ABCDEF'); | ||
| }).toThrow(new Error('Bad key')); | ||
| expect(new Cipher('ABCDEF')).toThrow(new Error('Bad key')); |
There was a problem hiding this comment.
You should revert this change. The toThrow matcher requires a function as an argument, but your change provides it with a Cipher object instead.
I imagine you were seeing an error for the eslint no-new rule. You can disable for this test block with:
/* eslint-disable no-new */
describe('Incorrect key cipher', function () {
...
});
/* eslint-enable no-new */There was a problem hiding this comment.
Oh good to know - thank you!
matthewmorgan
left a comment
There was a problem hiding this comment.
@leahzxxz A couple of small things, including some jshint cleanup that would be appreciated even though it wasn't your addition. :)
|
|
||
| function addEncodedCharacter(character, index, array) { | ||
| function addEncodedCharacter(character, index) { | ||
| /* jshint validthis:true */ |
There was a problem hiding this comment.
Hmm...I missed this the first time around. We should remove these jshint directives. I'm not sure if they were merged inadvertently or are artifacts of an older linting setup, but they don't make sense anymore.
| expect(function () { | ||
| new Cipher(''); | ||
| }).toThrow(new Error('Bad key')); | ||
| }); |
There was a problem hiding this comment.
Be sure to add the complementary enable line for the eslint rule.
There was a problem hiding this comment.
@matthewmorgan could you specify what that means? what is the complementary enable line? (sorry I'm still new to this 😃 )
There was a problem hiding this comment.
No problem.
You need to define the end of the disable block, otherwise the entire rest of the file will be ignored for that ESlint rule. EG:
/* eslint-disable no-new */
describe('Incorrect key cipher', function () {
...
});
/* eslint-enable no-new */Notice that this block begins with disable and ends with enable, which turns that linting rule back on.
There was a problem hiding this comment.
Thanks for explaining! Just pushed the change. Final issue woohoo!
|
|
||
| function addDecodedCharacter(character, index, array) { | ||
| function addDecodedCharacter(character, index) { | ||
| /* jshint validthis:true */ |
|
PS very excited to see that this is the final issue to resolve #399!! |
|
Nice work! One more question: do you feel comfortable squashing your work to a single commit? If not, I am happy to help with that. |
|
Hmm I haven't done it before - could you just squash and merge? Thanks! (would also appreciate if you let me know how to do it on my end) |
|
I can just squash and merge ;) You could do it like this: git checkout master
git pull upstream/master # or however you have named the exercism/javascript remote
git checkout fix/simple-cipher
git rebase master # this will replay your changes in a single commit onto masterAssuming you don't have any merge conflicts, just force-push your branch back up to your fork: git push -f origin fix/simple-cipherCare to give it a try? |
b42d379 to
3b931d7
Compare
|
@matthewmorgan thanks for the instructions! just rebased and fixed merge conflicts. Doesn't look like my commits are squashed though... Did I do it right 😝 |
|
One more step-- needed to make sure you were sync'd up with master (that was the hardest part.) Let's see if we can get this done before anyone merges anything else in ahead of us. git reset --soft fd567086481e7b85a5bbbb406b905acab72db6bb # last commit on master before your new ones
git commit -am 'Fix linting errors for simple-cipher'
git push -f origin fix/simple-cipher |
3b931d7 to
f13e19f
Compare
|
@matthewmorgan Done! let me know if you need anything else. Really appreciate all the guidance! |
|
No problem! Nice work, thanks for your contributions. |
Closes #399