Once an app is released, it is hard to change the API. We do control all the clients, but there is no guarantee that our users will upgrade their apps.

I'd like to propose that we use https://github.com/cookpad/globalweb-api-doc/projects/1 to:

• discuss, specify and share new or updated APIs before implementation
• co-ordinate API deprecations

By using a separate project,

• there is a shared place to discuss APIs across teams
• the app teams can subscribe to notifications without getting flooded by other global-web work
• we can co-ordinate gradual deprecation of APIs (and not forget!)

WDYT? @cookpad/global-web @cookpad/global-android @cookpad/global-ios

As an example, I've added an issue to handle the change proposed by @balvig in https://github.com/cookpad/global-web/pull/4196#issuecomment-260507403

• https://github.com/cookpad/globalweb-api-doc/issues/2

Which do we prefer and why and in which situation?

1. it "Includes the latest comment from the recipe author" do

2. it "includes the latest comment from the recipe author" do

3. example "Includes the latest comment from the recipe author" do

Should we update the guide based on https://github.com/cookpad/global-web/pull/12615#discussion_r282444010?

We allow this:

 def placeholder? 
   !!@placeholder 
end 

Related thread: https://github.com/cookpad/global-web/pull/13430/files#r301483447

While writing specs for a feature, I used a before do ... end to setup feature toggles and freeze time

https://github.com/cookpad/global-web/blob/110a2ec4ecc0ac81246dfbcb1633e761d7984d23/spec/events/avro/opened_contest_event_spec.rb#L5-L8

All the cases inside this spec file will test an event that is gonna be sent only if the feature toggle is enabled also in order to be tested properly we need to freeze time in each case

even though, the rules in our guide insist on not using context while writing specs https://github.com/cookpad/global-style-guides/tree/master/rspec#avoid-using-context
there are quite a few places where context is used in our project

as mentioned in the thread above:

I remember that

when we first wrote this guide it was based on this https://global.g.cookpad.com/qihuan-piao/20160512/171541#Good%20use%20case%20for%20let where it's stated that there is always a good use for contexts

however, the rule does not state that, can we please clarify this?

cc: @dannyd4315

@cookpad/web-chapter what do you think?

Upon running rubocop locally, I'm seeing this warning:

The following cops were added to RuboCop, but are not configured. Please set Enabled to either true or false in your .rubocop.yml file:

• Layout/SpaceAroundMethodCallOperator (0.82)
• Lint/RaiseException (0.81)
• Lint/StructNewOverride (0.81)
• Style/ExponentialNotation (0.82)
• Style/HashEachMethods (0.80)
• Style/HashTransformKeys (0.80)
• Style/HashTransformValues (0.80)
  For more information: https://docs.rubocop.org/en/latest/versioning/

We probably should look into what should be the values for those cops.

/cc @cookpad/web-chapter

I noticed in a PR from @lewispb, but didn't want to leave a comment there until we had established a rule, so here is an issue to talk about it.

There are multiple ways of passing a long list of parameters to a method:

The extra-long line

render CursorPaginator.new(records, cursor_field: cursor_field, cursor: params[:cursor], limit: params[:limit], meta: meta).response

This probably breaks the line length rule, and is not easily readable. It also makes it difficult to read the diff when a change happened to the line.

First parameter on the same line

render CursorPaginator.new(records,
                           cursor_field: cursor_field,
                           cursor: params[:cursor],
                           limit: params[:limit],
                           meta: meta).response

This is better, as it does not break the line length rule, and helps readability and diff.
It has the inconvenient of requiring a lot of indentation spaces, and also to realign the parameters if the method call (anything in render CursorPaginator.new() changes.

Indent all parameters once

render CursorPaginator.new(
  records,
  cursor_field: cursor_field,
  cursor: params[:cursor],
  limit: params[:limit],
  meta: meta
).response

This is my personal favorite. It has all the advantages of the previous, without the inconvenience. It also makes the chained call to .response a lot more visible.

Extra question: chaining

What do you guys think of chaining in the example above? Should the initialization be extracted to a private method? Should that be a rule too?
Example:

render cursor_paginator.response

# ...

def cursor_paginator
  @_cursor_paginator ||= CursorPaginator.new(
    records,
    cursor_field: cursor_field,
    cursor: params[:cursor],
    limit: params[:limit],
    meta: meta
  )
end

(Probably a bad example in that PR's context, as all params to the initializer need to be passed along...)

I think we can add some clarity to:

https://github.com/cookpad/guides/blob/master/code-review/README.md

After speaking with @balvig I will create a PR with some suggestions :)

Have always thought we discourage using before blocks in tests as we prefer explicitness , but it seems like that is not included in the list.

Ie something like this (work in progress):

Motivation

• I often find myself wanting to link to a specific item
• Having both reference links and links to code samples on the same item would be fiddly
• Maintaining code samples separately has always been a bit awkward

It will lose some of the compactness of the current version of course, but wondering what people think?

For workflow this first thing it says is:

Ensure pt-flow is installed and up to date 

Is this still true. I have never installed or used pt-flow so I have a feeling this is just arcane tooling.

At the moment, cookpad/guides is dominated by the team working on global-web. I think it would be helpful for the entire engineering team to consolidate here to discuss and document their workflow, coding style and general best practices.

This will especially be helpful for onboarding new engineers. A single repository should also be helpful for engineers who work on multiple technology stacks (iOS, Android, Web, ML, etc). Even for specialist engineers, I think it is also helpful to know how other technology teams operate for collaboration. e.g. if the iOS teams change their release cycle from 1 week to 2 weeks, etc.

What do you think?

• @cookpad/global-admin
• @cookpad/global-android
• @cookpad/global-ios
• @cookpad/global-ml
• @cookpad/global-search
• @cookpad/global-web

I hope I haven't missed anyone...

Next steps:

• adding guides for other technologies (iOS, Android, etc)

Some teams may not have well-formed best practices yet, and that's OK. I'd still like all teams to be involved!

Do we prefer bigint over integer?

Anything else we can document?

http://davidablack.net/dablog.html#2007/8/15/bang-methods-or-danger-will-rubyist

Reminds me I think maybe we should revisit this guide: https://github.com/cookpad/guides/tree/master/best-practices#avoid-trailing-conditional

Having tried out our current policy for a while, I think I've come around to liking @Knack's take on it:

"Avoid lines that end with conditionals except in cases where all expressions are extremely trivial"

#66 (comment)

~ @balvig https://github.com/cookpad/global-web/pull/8056#pullrequestreview-123295440

We mention using ♻️ and 👍 but we don't mention using [WIP] in the PR title, could be useful to document this.

Currently our guide is arranged into three folders:

• Workflow
• Code Review
• Best Practices
  • BEM

Primarily, however, this guide is used to refer to our agreed Ruby / Rails / JS / CSS style.

How about we move Workflow and Code Review elsewhere? Perhaps to the web chapter?

And then to make it easier to find the guide we are looking for we split out our current guides like this:

• Ruby
  • README.md
• Rails
  • README.md
  • rake.md
  • testing.md
  • i18n.md
• Javascript
  • README.md
• CSS
  • README.md

Re-post from https://ckpd.slack.com/archives/web-dev/p1488386407163541 for better logging.

We have started “sprint” development. Since now multiple teams are working simultaneously, we become to suffer staging resource now.
So I’d like to propose to keep deploying 1 dedicated branch to staging-mobile.

1. The first person who wants to deploy own branch creates a new branch staging_mobile staging_mobile_[week]_sprint (e.g. staging_mobile_w10_sprint) from master and merge own branch to it
2. Then, when other person wants to deploy own branch, they merge own to staging_mobile staging_mobile_w10_sprint

If there's no concern, I'd like you to follow this from now 🙏

This PR adding Phyton made me realize that we have been using this repository to keep style guides for the web chapter. Since that's no longer the case maybe we should move some of the guidelines to some folder to make their scope clearer.

That would mean:

• Moving testing/README.md to rails/testing/README.md or even ruby/testing/README.md
• Moving rake/README.md to rails/testing/README.md

What do you think?