Skip to content

Inactives language fix#846

Merged
homu merged 3 commits intonew-xkit:masterfrom
ChuckL:inactives_language_fix
Dec 23, 2015
Merged

Inactives language fix#846
homu merged 3 commits intonew-xkit:masterfrom
ChuckL:inactives_language_fix

Conversation

@ChuckL
Copy link

@ChuckL ChuckL commented Dec 6, 2015

Fixed #833

If the user had a different language than English it would fail.

Copy link

Choose a reason for hiding this comment

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

We can leave this out.

Copy link
Member

Choose a reason for hiding this comment

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

would prefer to do peopleCount = parseInt(count_text.replace(etc)) so that the operations are self-documenting

@ChuckL
Copy link
Author

ChuckL commented Dec 9, 2015

@nightpool I think it's fine, it's one logical step per line rather than nested method calls.
I could make another variable to make it easier to read / more documentation if you think that's better.
I could also do something like var count_text = $("#tabs").html().replace(/[^0-9]/g, '')

@bvtsang I opted to have some duplicated code since I think it's easier to read and understand than what I would do probably do otherwise. Make an enum type object and loop through the enums.

@nightpool
Copy link
Member

@ChuckL but turning it from the string into the int is a logical operation, right? and that requires dealing both with the useless text and the parseInt function.

I agree with keeping the duplicated code easier to read btw @bvtsang @ChuckL

@ChuckL
Copy link
Author

ChuckL commented Dec 9, 2015

Turning a string to an int in a logical operation.

destroying useless text text then turning it into an interger, in my mind isn't a single logical operation which is shown by the multiple method calls in order to do that.

@nightpool
Copy link
Member

down for whatever tbh. its a minor nitpick

On Wed, Dec 9, 2015 at 12:28 AM Charles notifications@github.com wrote:

Turning a string to an int in a logical operation.

destroying useless text text then turning it into an interger, in my mind
isn't a single logical operation.


Reply to this email directly or view it on GitHub
#846 (comment).

@ChuckL
Copy link
Author

ChuckL commented Dec 23, 2015

So is this good to go or are there a few bugs / styling stuff still?

@hobinjk
Copy link

hobinjk commented Dec 23, 2015

Tested locally, LGTM. I don't think any of the remaining nitpicks are blockers.
@homu r+

@homu
Copy link

homu commented Dec 23, 2015

📌 Commit ba15e0d has been approved by hobinjk

homu added a commit that referenced this pull request Dec 23, 2015
Inactives language fix

Fixed #833

If the user had a different language than English it would fail.
@homu
Copy link

homu commented Dec 23, 2015

⌛ Testing commit ba15e0d with merge 7ac8167...

@homu
Copy link

homu commented Dec 23, 2015

☀️ Test successful - status

@homu homu merged commit ba15e0d into new-xkit:master Dec 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants