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

Update API/generate, add Russian #733

Closed
wants to merge 1 commit into from
Closed

Conversation

qm3ster
Copy link

@qm3ster qm3ster commented Aug 25, 2018

No description provided.

Also added Russian
Copy link
Member

@Atinux Atinux left a comment

Choose a reason for hiding this comment

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

Thanks @qm3ster

Could you look at my comments please?

})
}
routes: () =>
axios.get('https://my-api/users').then(res =>
Copy link
Member

Choose a reason for hiding this comment

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

return statement missing

Copy link
Author

Choose a reason for hiding this comment

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

What, where?

'/users/2',
'/users/3'
]
routes: ['/users/1', '/users/2', '/users/3']
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes it clearer

Copy link
Author

Choose a reason for hiding this comment

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

It was Prettier that did this, actually. Seemed more than short enough not to split.
I split the french because it uses /utilisateurs/ instead of /users/ which imo is a bit much. Maybe the examples should be monolingual, so that if we later have a translation build process they can be imported from one place.

if (payload) return { user: payload }
else return { user: await backend.fetchUser(params.id) }
}
asyncData: async ({ params, error, payload }) =>
Copy link
Member

Choose a reason for hiding this comment

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

While it might be optimised, I don't think it's a good idea for documentation since it makes it harder to read for a human :)

Copy link
Author

Choose a reason for hiding this comment

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

  1. I like to think that it's more clear that the function doesn't rely on this when it's an arrow.
  2. If I put it as method syntax, I'd have to have a return statement, which dilutes the actual logic of the 2 paths.
  3. Clearly, I am sane and even conservative, since I had the sense to see the current solution is more readable to a novice dev than:
    asyncData: async ({ params, error, payload }) => ({
      user: payload || (await backend.fetchUser(params.id))
    })

and stopped at that.

That said, I'm open to change this and go through all the files substituting it, just tell me which specific operator you dislike here.

Copy link
Member

Choose a reason for hiding this comment

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

Would like @manniL opinion on this particular comment

Copy link
Author

Choose a reason for hiding this comment

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

@manniL it has come a full circle UwU

Copy link
Member

Choose a reason for hiding this comment

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

I'd go for something like

async asyncData ({ params, error, payload }) {
  if (payload) {
    return { user: payload }
  }
  const user = await backend.fetchUser(params.id)
  return { user }
}

Not all readers know ES6 and short circuits very well, so I'd favor a "more readable" /simple version

Copy link
Author

Choose a reason for hiding this comment

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

Oke, will do.
How do you feel about the original version:

async asyncData ({ params, error, payload }) {
  if (payload) return { user: payload }
  else return { user: await backend.fetchUser(params.id) }
}

i'd just change it to arrow notation since this isn't in use, and remove the unnecessary else

asyncData: async ({ params, error, payload }) => {
  if (payload) return { user: payload }
  return { user: await backend.fetchUser(params.id) }
}

Copy link
Member

Choose a reason for hiding this comment

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

@qm3ster I don't like "inline ifs" actually. Was a fan of them years ago but now I appreciate the extra braces for readability.

This is also aligns with our eslint config.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha, will do like yours. Should the await also be on a separate line?
whispers: multiline ifs are a code smell, pull them out into a named function

@qm3ster
Copy link
Author

qm3ster commented Aug 26, 2018

@Atinux I replied to the comments, tell me what to do please :)

@manniL
Copy link
Member

manniL commented Dec 12, 2018

cc @Atinux

.then((res) => {
var routes = res.data.map((user) => {
return '/users/' + user.id
routes: callback =>
Copy link
Author

@qm3ster qm3ster Jan 29, 2019

Choose a reason for hiding this comment

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

Actually @manniL I now see that this may be confusing, because it does call the callback, but also returns a Promise which resolves after the callback returns.
Would putting void axios.get be too arcane?
Should I just put the statement of the routes function into a {} block so that it returns undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want to keep a callback example for the routes function at all.

Copy link
Author

Choose a reason for hiding this comment

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

That's fine by me. 😩 👌
So I nuke it?
From all languages?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say so

@Atinux Atinux closed this May 25, 2020
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.

None yet

3 participants