-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix(git): branches and pull-request #94
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¿Y qué hacemos con los chore(): bump version
que hacemos para las app móviles?
@llekn cómo funcionan esos commits? |
@bunzli Mas o menos así:
Igual es una mecánica un poco apestosa, manual y propensa a errores. Feliz si encontramos una mejor manera de lograr lo mismo (tags, version bump en el pipeline de CI, etc) |
@llekn y por qué no se hace esto con un PR? |
Yo creo que principalmente porque es un poco engorroso, pero mi argumento es débil porque el PR está como a 2 clicks de distancia de sólo el commit (y una revisión, quizás eso es lo más doloroso pero podría obviarse en este caso). Si no nos ponemos nazis con la revisión del PR en este caso, no veo por qué no hacerlo como propone usted, señor @bunzli . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es medio latero hacer PRs de ese caso, pero en realidad sí se podrían hacer para ser consistentes. Así que aprobado 🏅
Aprobado considerando la posibilidad de "self-merge"? |
Yo sería talibán no más! ¿Quien quiere ganarse unos froggo points gratis? y
listo, aprobado en pocos segundos.
…On Wed, Mar 27, 2019 at 11:22 AM Camilo Flores ***@***.***> wrote:
Aprobado considerando la posibilidad de "self-merge"?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#94 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAtQa4AWe_OI31gisfXkhDXypbgL4aV5ks5va36cgaJpZM4cHdkv>
.
--
Jaime Bünzli
*platanus <http://platan.us>*
|
@bunzli una duda con lo de feature branch: cuando se haca el gran merge a master habría que pedir code review de nuevo no? Entonces me revisarían ese código dos veces? Y otra cosa, quizás en esta sección (o en otra no se), se podría agregar algo del objetivo de froggo, lo de repartir los PRs |
Muy buenos puntos. Creo que va a quedar medio duplicado el review, pero seguramente el PR va a ser bien grande y va a tender a una revisión del estilo LGTM. Tengo pensado agregar algo de froggo (lo anoté como pega de viernes) en la guía. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull reminder me recuerda que nunca aprobé esto explícitamente 😅
@bunzli quieres arreglar esos conflictos? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tal vez sería bueno agregar que recomendamos borrar el branch una vez que se mergea.
Se cambia la sección de Branches y PR en donde queda interpretable que podemos hacer commits directamente en master.