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

Add linting with Rubocop, generated docs with Yard #14

Merged
merged 48 commits into from
Apr 19, 2023
Merged

Add linting with Rubocop, generated docs with Yard #14

merged 48 commits into from
Apr 19, 2023

Conversation

tinahollygb
Copy link
Contributor

@tinahollygb tinahollygb commented Apr 19, 2023

Docs

Generates class docs. On a successful push to main, they build and deploy here: https://growthbook.github.io/growthbook-ruby

Removes redundant docs from the readme (it's all duplicated from the real docs).

Related PR links to this new generated class docs site: growthbook/growthbook#1202

Dead code

Deletes deprecated classes Client, Experiment, ExperimentResult, User, LookupResult.

Linting, Tests, CI

Adds linting with Rubocop. This required a lot of changes for naming conventions and style.

On CI, we are now running:

  • lint, on Ruby 3.2.2 (newest)
  • tests on:
    • 2.5 – oldest we can support based on syntax
    • 3.2.2 – newest version of Ruby

Test coverage

Also adds code coverage. Reports can be open locally by running open coverage/index.html after running bundle exec rspec.

image

image

def self.chooseVariation(userId, experiment)
testId = experiment.id
weights = experiment.getScaledWeights
def self.choose_variation_for_user(user_id, experiment)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already a choose_variation so I needed to call this one something else to avoid a naming conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got cleaned up when deleting the deprecated classes. 🤷‍♀️

Comment on lines 5 to 26
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: 3.2.2
bundler-cache: true
- name: Lint
run: |
gem install rubocop -v 1.50.2
gem install rubocop-rspec -v 2.20.0
rubocop

build-test:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
ruby:
- '2.5'
- '3.2.2'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On CI, we are now running:

  • lint, on Ruby 3.2.2 (newest)
  • tests on:
    • 2.5 – oldest we can support based on syntax
    • 3.2.2 – newest version of Ruby

Comment on lines 8 to 25
docs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: 3.2.2
bundler-cache: true
- name: Generate docs
run: |
gem install yard
yard doc 'lib/**/*.rb'

- name: Publish docs
uses: peaceiris/actions-gh-pages@v3
with:
GITHUB_TOKEN: ${{ github.token }}
publish_dir: doc
Copy link
Contributor Author

Choose a reason for hiding this comment

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


## Quick start
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These docs are redundant so I'm linking to the official docs instead.

Comment on lines -138 to +141
return nill unless val
return nil unless val
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch must be untested because this should've thrown a reference error.

@tinahollygb tinahollygb merged commit c109de0 into main Apr 19, 2023
@tinahollygb tinahollygb deleted the lint branch April 19, 2023 19:30
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.

2 participants