Code cleanliness improvements #1192

Merged
merged 38 commits into from Aug 18, 2016

Conversation

Projects
None yet
4 participants
@Calvinp
Contributor

Calvinp commented Aug 5, 2016

  • Renames most one and two letter variable names to things that are more descriptive.
  • Adds an eslint rule to disallow most one and two letter variable names. id, ui, and s3 are ok because they are useful and informative when they are used, unlike hc (for healthCheck)
  • Adds a block exception to this rule for the moment locale rules.
  • Fixes a spot where props were mutated.
  • Uses moment.updateLocale() instead of moment.locale() to get rid of the deprecated warning.
  • Makes several components functional.

Calvinp added some commits Aug 5, 2016

Eslint disallow one-letter and most two-letter variable names. 'id', …
…'ui', and 's3' are ok because they are useful and informative when they are used, unlike 'hc' (for healthchecks)
@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Aug 5, 2016

Contributor

I always forget to do this... cc @tpetr @kwm4385 @wolfd

Contributor

Calvinp commented Aug 5, 2016

I always forget to do this... cc @tpetr @kwm4385 @wolfd

@Calvinp Calvinp referenced this pull request Aug 9, 2016

Merged

Request groups page #1196

@Calvinp Calvinp referenced this pull request Aug 10, 2016

Closed

Cleanup Webhooks Page #1201

@@ -29,7 +29,7 @@ export default class Duration extends React.Component {
min={0}
step={1}
value={this.props.value ? duration.hours() : ''}
- onChange={(e) => this.handleChange(e)}
+ onChange={(event) => this.handleChange(event)}

This comment has been minimized.

@kwm4385

kwm4385 Aug 10, 2016

Contributor

These could probably be shortened just to onChange={this.handleChange}

@kwm4385

kwm4385 Aug 10, 2016

Contributor

These could probably be shortened just to onChange={this.handleChange}

This comment has been minimized.

@Calvinp

Calvinp Aug 10, 2016

Contributor

When making this change, I found a bug (that is present in decaf too). I'm working on fixing that.

@Calvinp

Calvinp Aug 10, 2016

Contributor

When making this change, I found a bug (that is present in decaf too). I'm working on fixing that.

+const Racks = (props) => {
+ const showUser = (rack) => Utils.isIn(rack.currentState.state, ['ACTIVE', 'DECOMMISSIONING', 'DECOMMISSIONED', 'STARTING_DECOMMISSION']);
+
+ const getMaybeReactivateButton = (rack) => (Utils.isIn(rack.currentState.state, ['DECOMMISSIONING', 'DECOMMISSIONED', 'STARTING_DECOMMISSION']) &&

This comment has been minimized.

@kwm4385

kwm4385 Aug 10, 2016

Contributor

Might want to split these into their own functions, its difficult to follow what's going on here.

@kwm4385

kwm4385 Aug 10, 2016

Contributor

Might want to split these into their own functions, its difficult to follow what's going on here.

if (pausedRequests.length > 0) {
pausedRequestsSection = (
<UITable
data={pausedRequests}
- keyGetter={(r) => r.request.id}
+ keyGetter={(request) => request.request.id}

This comment has been minimized.

@wolfd

wolfd Aug 17, 2016

Contributor

these are actually requestParents, but we don't do a good job of labeling them...

@wolfd

wolfd Aug 17, 2016

Contributor

these are actually requestParents, but we don't do a good job of labeling them...

This comment has been minimized.

@Calvinp

Calvinp Aug 17, 2016

Contributor

Yeah, I'm realizing that. I'm willing to go through and change them all - it's probably worth doing.

@Calvinp

Calvinp Aug 17, 2016

Contributor

Yeah, I'm realizing that. I'm willing to go through and change them all - it's probably worth doing.

SingularityUI/app/selectors/requests.es6
@@ -1,7 +1,6 @@
import { createSelector } from 'reselect';
import micromatch from 'micromatch';
import fuzzy from 'fuzzy';
-import _ from 'underscore';

This comment has been minimized.

@tpetr

tpetr Aug 17, 2016

Member

i'd like to move away from global imports, not towards -- can we add this back?

@tpetr

tpetr Aug 17, 2016

Member

i'd like to move away from global imports, not towards -- can we add this back?

SingularityUI/app/utils.es6
- return a + b;
+ joinPath(firstPart, secondPart) {
+ if (!firstPart.endsWith('/')) firstPart += '/';
+ if (secondPart.startsWith('/')) secondPart = secondPart.substring(1, secondPart.length);

This comment has been minimized.

@wolfd

wolfd Aug 17, 2016

Contributor

substring has the second arg optional if you want it to go to the end.

@wolfd

wolfd Aug 17, 2016

Contributor

substring has the second arg optional if you want it to go to the end.

@tpetr tpetr merged commit 1c7a8c3 into decaf Aug 18, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@tpetr tpetr modified the milestone: 0.10.0 Aug 18, 2016

@ssalinas ssalinas deleted the code_cleanliness_improvements branch Nov 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment