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

Merge rules and recommendations from betterspecs.org #55

Merged
merged 18 commits into from
Nov 7, 2018
Merged

Conversation

pirj
Copy link
Member

@pirj pirj commented Nov 2, 2018

https://github.com/lelylan/betterspecs are not actively maintained, but
contain a lot of useful material that fits this style guide quite well.
The license of Betterspecs allow for derivative work, and there were no
objections
on
picking the material to this guide.

What has been done

  • merge all gh-pages
  • check changes on master (diverged from gh-pages)
  • check open pulls
  • improved the wordings, code examples and updated to use the day
  • consider the differences between the STYLE and HELPFUL RECOMMENDATIONS (the way it's done in the other guides)
  • check for html tags (e.g. <code>, <p>)
  • mention fivebar (along with fuubar)
  • attribute Andrea as the author of every commit
  • add credit to Andrea and Lelylan to the docs
  • squash fixups

What has not been done

Links to commits, one rule per commit

Open questions

1. Should we merge as one change, or should I better split them into
separate pull requests?
Merging as one should be good.

  1. Should we reference the discussions behind the superseded topics
    (listed above in "What has not been done" section) somehow?

Copy link
Member

@Darhazer Darhazer left a comment

Choose a reason for hiding this comment

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

Great job!

README.md Outdated Show resolved Hide resolved
Copy link

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

I’m just a few commits into this pull request, and I already disagree with a whole lot of things :-) (I’ll just leave 2 of my inline comments)

Would you prefer that we discuss all issues in this PR, or should we merge it as-is and discuss changes in separate issues afterwards?

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@pirj
Copy link
Member Author

pirj commented Nov 2, 2018

@bquorning I feel the same pain, but I don't feel I could abuse my style guide editor permissions to only selectively pick or to modify those rules on my own when they come from a guide with over 3000 stars.

Let's start the discussion here, and see how it goes.

@benoittgt
Copy link

Thanks a lot !

@pirj pirj force-pushed the merge-betterspecs branch from a53bdcf to 1204694 Compare November 7, 2018 20:31
@pirj pirj force-pushed the merge-betterspecs branch from 1204694 to bd02acf Compare November 7, 2018 20:34
@pirj pirj merged commit ad04f97 into master Nov 7, 2018
@pirj pirj deleted the merge-betterspecs branch November 7, 2018 20:35
@pirj pirj mentioned this pull request Nov 7, 2018
Copy link

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

I am sorry @pirj, I forgot to review the rest of your PR in due time. Turned out I had a few extra comments that I hope it’s ok to add here. I think opening a new issue for each of them is overkill, at least initially.

README.md Show resolved Hide resolved
are not nearly as maintainable as factories.
Use [Factory Bot](https://github.com/thoughtbot/factory_bot) to
create test data in integration tests. You should very rarely
have to use `ModelName.create` within an integration spec. Do

Choose a reason for hiding this comment

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

I believe the ModelName.create example is specific to Rails. Should we consider moving this section down to Rails - models section, or perhaps just add a note in the text?

Copy link
Member Author

Choose a reason for hiding this comment

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

The wording implies Rails, but the advice itself is quite generic. Fixtures here are some objects kept in an storage, not even necessarily database. Also:

Although factory_bot is written to work with ActiveRecord out of the box, it can also work with any Ruby class.

I'd rather keep the rule in this section, but changed ModelName.create into something that applies both to ActiveRecord and, say, file system.

README.md Show resolved Hide resolved
@pirj
Copy link
Member Author

pirj commented Nov 8, 2018

@bquorning thanks for the review, it's better late than never 👍

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.

5 participants