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

Fix challenges order #101

Merged
merged 2 commits into from Nov 1, 2013
Merged

Conversation

KristianTashkov
Copy link
Contributor

Sort challenges by ascending create date so it matches the order in tasks.

@mitio
Copy link
Collaborator

mitio commented Nov 1, 2013

Така нещата ми изглеждат доста по-добре :) Аз лично не държа да squash-ваме commit-ите, но @skanev спомена нещо?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d2dac1d on KristianTashkov:fix-challenges-order into bb8c6dd on skanev:master.

This is done to match the same order used in tasks.
@KristianTashkov
Copy link
Contributor Author

Върнах старите имена по желание на Стефан, оправих индентацията и squash-нах всичко до един commit защото вече нямаше смисъл от останалите.

@mitio
Copy link
Collaborator

mitio commented Nov 1, 2013

Върнах старите имена по желание на Стефан

Къде е станала тази дискусия?

@KristianTashkov
Copy link
Contributor Author

Върху commit-a който изтрих защото revert-нах промените му (преименоването) и само замърсяваше историята. Аргумента му беше че за тест с повече от 2 challenge-а няма подходящо име за третия. Освен това явно е конвенция в проекта да се именоват подобни ситуации first/second.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e165325 on KristianTashkov:fix-challenges-order into bb8c6dd on skanev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e165325 on KristianTashkov:fix-challenges-order into bb8c6dd on skanev:master.

@mitio
Copy link
Collaborator

mitio commented Nov 1, 2013

@KristianTashkov Мерси за информацията. Това е добра бележка, между другото, какви са недостатъците да се коментира на commit, а не в diff-а тук и какви са недостатъците да се squash-ва. В сходна ситуация в бъдеще, аз не бих squash-нал, за да запазя тази дискусия и аргументация там.

Иначе, мисля, че сме окей за merge. Нека @skanev има честта :) А аз ще се разплатя с теб формално.

it "can fetch all visible records, sorted in reverse chronological order" do
second = create :visible_challenge, created_at: 3.days.ago
it "can fetch all visible records, sorted in chronological order" do
first = create :visible_challenge, created_at: 3.days.ago
Copy link
Owner

Choose a reason for hiding this comment

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

Тук си развалил идентацията ;D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

преди it-а са 2 , преди first 4 space-a, това е ок? За това че равното не е подравнено с долното ли говориш?

Copy link
Owner

Choose a reason for hiding this comment

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

За същото. Било е така, няма нужда да го убиваш :)

@KristianTashkov
Copy link
Contributor Author

Оправих я :)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a142693 on KristianTashkov:fix-challenges-order into bb8c6dd on skanev:master.

@skanev
Copy link
Owner

skanev commented Nov 1, 2013

Перфектно 👍

skanev added a commit that referenced this pull request Nov 1, 2013
@skanev skanev merged commit a8a82c8 into skanev:master Nov 1, 2013
@KristianTashkov KristianTashkov deleted the fix-challenges-order branch November 2, 2013 08:11
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

4 participants