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

ES6: migrated "user" #1138

Merged
merged 41 commits into from
May 3, 2017
Merged

ES6: migrated "user" #1138

merged 41 commits into from
May 3, 2017

Conversation

bennycode
Copy link
Contributor

Type of change

Screenshot


for (let i = 0; i < additional_numbers; i++) {
random_digits.push(z.util.get_random_int(1, 9));
}
Copy link
Contributor

@lipis lipis Apr 28, 2017

Choose a reason for hiding this comment

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

const random_digits = [...Array(additional_numbers).keys()].map(() => z.util.get_random_int(1, 9));

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this 9 should be a const..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please read the paragraph "Extra code is not the enemy" from: https://dzone.com/articles/using-java-8-please-avoid-functional-vomit

Copy link
Contributor

Choose a reason for hiding this comment

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

but less code sometimes it is :) rename i --==> index

function generate_handle_variations(handle, number_of_variations = 5) {
return new Array(number_of_variations).map(function(value, i) {
i += 1;
return append_random_digits(handle.slice(0, MAX_HANDLE_LENGTH - i), i);
Copy link
Contributor

Choose a reason for hiding this comment

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

index instead if i

}
}

get_connections(limit = 500, user_id, connection_ets = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number.. maybe DEFAULT_CONNECTION_LIMIT

@bennycode bennycode changed the title 🚧 ES6: migrated "user" ES6: migrated "user" May 1, 2017
Copy link
Contributor

@lipis lipis left a comment

Choose a reason for hiding this comment

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

Change single letter variables to something more meaningful.

Eventually we should introduce this rule as well.. http://eslint.org/docs/rules/id-length

@bennycode
Copy link
Contributor Author

@lipis: Let's make a deal... I will change i to index and you will submit an ESLint PR with an id-length rule, ok? :D

@lipis
Copy link
Contributor

lipis commented May 2, 2017

OKi.. but I'll do it later tonight :) as it will be a fun thing to fix all of them! Either case... deal!

break;
default:
this.message = 'Unknown UserError';
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

No nead to break on default

* @returns {Array<string>} Handle variations
*/
function generate_handle_variations(handle, number_of_variations = 5) {
return new Array(number_of_variations).map(function(value, index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to work. Map will skip but return undefined in an array: http://2ality.com/2013/11/initializing-arrays.html

return _.range(number_of_variations).map(function(value) {
  return append_random_digits(handle.slice(0, MAX_HANDLE_LENGTH - value), value);
}

function append_random_digits(handle, additional_numbers) {
const random_digits = [];

for (let index = 0; index < additional_numbers; index++) {
Copy link
Contributor

@gregor gregor May 2, 2017

Choose a reason for hiding this comment

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

Simpler:

const random_digits = _.range(additional_numbers).map(() => z.util.get_random_int(1, 9));

Copy link
Contributor

Choose a reason for hiding this comment

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

we already discuss that one with @bennyn as I also suggested something similar.. and we agreed that it's not the most obvious thing :)


if (data.service) {
user_et.is_bot = true;
user_et.provider_id = data.service.provider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot to use destructuring:

if (data.service) {
  const {provider: provider_id, id: service_id} = data.service

  user_et.is_bot = true;
  user_et.provider_id = provider_id;
  user_et.service_id = service_id;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer two lines over three lines:

user_et.provider_id = data.service.provider;
user_et.service_id = data.service.id;

vs.

const {provider: provider_id, id: service_id} = data.service
user_et.provider_id = provider_id;
user_et.service_id = service_id;

Especially because provider_id and service_id are not used anywhere else in this function.

this.connect_requests = ko.pureComputed(() => {
return this.users()
.filter((user_et) => user_et.connection().status() === z.user.ConnectionStatus.PENDING)
.map((user_et) => user_et);
Copy link
Contributor

Choose a reason for hiding this comment

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

What are you using map for here? Line seems obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am filtering undefined items with it.

initiate_password_reset(email, phone_number) {
return this.client.send_json({
data: {
email,
Copy link
Contributor

Choose a reason for hiding this comment

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

email: email

*/
update_own_user_profile(data) {
return this.client.send_json({
data,
Copy link
Contributor

Choose a reason for hiding this comment

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

data: data,

change_own_email(email) {
return this.client.send_json({
data: {
email,
Copy link
Contributor

Choose a reason for hiding this comment

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

email: email,

change_own_password(new_password, old_password) {
return this.client.send_json({
data: {
new_password,
Copy link
Contributor

Choose a reason for hiding this comment

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

new_password: new_password,
old_password: old_password,

*/
delete_self() {
return this.client.send_json({
data: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Has anyone ever checked why we send this data content? Seems like an obsolete placeholder that should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am just a messenger here... So should we change this code @herrmannplatz?

  delete_self() {
    return this.client.send_json({
      data: {
        todo: 'Change this to normal request!',
      },
      type: 'DELETE',
      url: this.client.create_url(UserService.URL.SELF),
    });
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

@lipis build it inititally

@bennycode bennycode merged commit 48e5067 into dev May 3, 2017
@bennycode bennycode deleted the es6-user branch May 3, 2017 09:35
@lipis lipis mentioned this pull request May 3, 2017
79 tasks
lipis added a commit that referenced this pull request May 3, 2017
* 'eslint' of github.com:wireapp/wire-webapp:
  Updated translations (#1168)
  Skipping HTML comments (#1159)
  ES6: migrated "user" (#1138)
  Fix linting
  Return Object in ClientMapper.update_client() (#1158)
  chore(package): update eslint-plugin-jsdoc to version 3.0.2 (#1164)
  dont display blocked top people (#1163)
  Per-user count-based alphabetic emoji ordering (#1126)
  Fix removeEventListener on mouse click, fix emoji popup reappearing after a typo (#1133)
  Fix emoji flags detection (#1162)
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.

4 participants