Skip to content
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.

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

Cleans '/src/utils' through 'interpolation.js'. #1018. Also #708. #1037

Merged
merged 4 commits into from
Dec 12, 2018

Conversation

knod
Copy link
Collaborator

@knod knod commented Dec 2, 2018

No description provided.

@knod knod added this to To do in 3:2 Documentation via automation Dec 2, 2018
@knod knod requested a review from turnerhayes December 2, 2018 13:08
@knod knod added this to To do in 0:1 Linting and code style via automation Dec 2, 2018
@knod knod requested a review from dylanesque December 2, 2018 13:08
// @todo get this value from the app somewhere
USState: 'MA', // Two-letter code denoting the state the client is in
// @todo get this value from the app somewhere
USState: `MA`, // Two-letter code denoting the state the client is in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to look into ESLint config option spacing? to fix this. I'm reluctant to disable ESlint for the whole file; at most, I hope we could disable just a specific rule to fix this, preferably in a block (/* eslint-disable */ ... /* eslint-enable */) rather than whole-file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spacing rules are the thing that's causing the problem. I didn't find any spacing rules that would be more useful in there. I disabled them in this block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you disable only the rule that's causing the problem in that block, rather than a total disabling of ESLint? Also add a comment explaining why it's being disabled in that block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've found a way to disable for a specific rule. I'm going to change this up, though, and disable the rule for the whole object. If/when new values are added, I don't want them to have the same unexpected result as before and have to figure it out all over again.

This also makes me question whether this column thing should be around at all. Maybe we remove the column headings and just go back to regular spacing rules. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of agree about the column headings--I see their value, but we could probably accomplish the same thing without them (I don't think it's too confusing what part of the code is what without the columns).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll just get rid of them then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's done. Is this ready for merging?

Copy link
Collaborator

@dylanesque dylanesque left a comment

Choose a reason for hiding this comment

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

What's the purpose of this forUpdate function in convertForUpdate.js? All it seems to be doing is returning itself?

@knod
Copy link
Collaborator Author

knod commented Dec 8, 2018

@dylanesque : I don't see a forUpdate function, just an object. Are we talking about the same thing?

@turnerhayes
Copy link
Collaborator

@dylanesque are you referring to this?:

const convertForUpdate = function ({ name, route, ...otherProps }) {
   let forUpdate = {
     ...otherProps,
     route: route || name,
   };
   return forUpdate;
 };  // End convertForUpdate()

@dylanesque
Copy link
Collaborator

dylanesque commented Dec 10, 2018

@dylanesque are you referring to this?:

const convertForUpdate = function ({ name, route, ...otherProps }) {
   let forUpdate = {
     ...otherProps,
     route: route || name,
   };
   return forUpdate;
 };  // End convertForUpdate()

That's what I meant, but the quoted code you have above doesn't match at all what I'm seeing in the PR, namely:

   let forUpdate = {
       return forUpdate;	 
 };
 export { convertForUpdate };	export { convertForUpdate };

@knod
Copy link
Collaborator Author

knod commented Dec 10, 2018

@dylanesque : that's just github skipping lines - it avoids showing the whole file and concentrates on what's been changed. If you see, there's a line highlighted in blue that has icons at the left edge. If you click on those, it'll expand to show more of the code. You can also click 'view' for the file and see the new version in full.

@knod knod merged commit e4d5566 into codeforboston:dev Dec 12, 2018
0:1 Linting and code style automation moved this from To do to Done Dec 12, 2018
3:2 Documentation automation moved this from To do to Done Dec 12, 2018
@knod knod deleted the code-style-14 branch December 12, 2018 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants