Skip to content
This repository has been archived by the owner on Apr 11, 2018. It is now read-only.

Add Array#fetch example with block #72

Merged
merged 1 commit into from Oct 23, 2014
Merged

Add Array#fetch example with block #72

merged 1 commit into from Oct 23, 2014

Conversation

ymihaylov
Copy link
Contributor

screenshot from 2014-10-16 13 01 39

Видях този начин на употребва на fetch в Stack Overflow след което и в документацията и реших да го добавя и в нашата презентация, защото би могло да бъде интересно и полезно за хората, които ще преглеждат отново лекцията.

@mitio
Copy link
Member

mitio commented Oct 16, 2014

Това е добра промяна, но имам нужда от допълнителна информация, преди да я приема.

Добре е да сложиш screenshot на целия слайд, когато промяната не е дребна корекция. Тук ти си разместил примери и си добавил нови. Ще ми помогне много, ако има screenshot на текущата версия.

Допълнително нещо, което е добре да включиш в pull request-а, е мотивация защо си избрал да направиш тези корекции, защо смяташ, че трябва да ги има в слайда.

Тези неща е полезно да присъстват в описанието на pull request-а принципно, особено мотивацията, ако правиш нещо повече от поправка на очевидна (например правописна) грешка. Ще ускорят приемането на предложените промени и ще спестят време на този, който ги преглежда.

@ymihaylov
Copy link
Contributor Author

Updated! ;)

@mitio
Copy link
Member

mitio commented Oct 23, 2014

Така е супер.

Имам само още една забележка – според упътванията за принос, които имаме в README-то на хранилището, branch-ът е добре да се казва 03-fetch-example, вместо само fetch-example, защото е по-ясно за коя лекция става въпрос. Даже аз бих го кръстил 03-fetch-with-block.

Тъй като това е дребно и не пречи на функционалността, го отбелязвам, за да го имаш предвид занапред. Приемам промяната и плащам с точка. Благодаря :)

mitio added a commit that referenced this pull request Oct 23, 2014
Add Array#fetch example with block
@mitio mitio merged commit f7c2586 into fmi:master Oct 23, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants