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

Updates for ruby 3.0 #1

Merged
merged 26 commits into from
Jul 5, 2024
Merged

Updates for ruby 3.0 #1

merged 26 commits into from
Jul 5, 2024

Conversation

rortian
Copy link
Collaborator

@rortian rortian commented Jul 1, 2024

No description provided.

@rortian
Copy link
Collaborator Author

rortian commented Jul 1, 2024

Need to add some ruby versions to test matrix

@@ -0,0 +1,17 @@
## What is the current behavior?
Copy link

Choose a reason for hiding this comment

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

where did these come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cp -r ../jsonapi.rb .

@@ -0,0 +1,29 @@
name: CI

on: [push, pull_request]
Copy link

Choose a reason for hiding this comment

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

similar to the cequel fork -- should we make this push on main + master branches only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm willing to add or delete

Copy link

Choose a reason for hiding this comment

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

I guess Tate can confirm -- I think the rationale was so that our test suite doesnt run tests twice


strategy:
matrix:
ruby: ['2.7', '3.0', '3.1']
Copy link

Choose a reason for hiding this comment

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

Should we add rails versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No rails in the tests


- name: Runs code QA and tests
env:
RAILS_VERSION: ~> ${{ matrix.rails }}
Copy link

Choose a reason for hiding this comment

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

where does this come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

someone named Tate

Copy link

Choose a reason for hiding this comment

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

But there is no rails in the matrix right? or in the tests? maybe this isnt needed

Copy link

Choose a reason for hiding this comment

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

bump -- isnt this not needed since no rails?

@lexahu
Copy link

lexahu commented Jul 2, 2024

Similar to the other forks -- I think well need to add a rake task to publish the gem version to our repo
like
https://github.com/art19/jsonapi.rb/pull/5/files

rortian and others added 3 commits July 2, 2024 13:56
@@ -37,11 +37,20 @@ module Protocol
end

describe('RUBY-128') do
let(:permitted_classes) { [Cassandra::Protocol::CqlByteBuffer, Cassandra::Types::Map, Cassandra::Types::Simple, Symbol] }

Choose a reason for hiding this comment

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

total nit pick, feel free to ignore, but I'd make this a variable inside the if Psych branch since it's only used there. Generally you reserve let blocks for setup or assertions across multiple specs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to ignore, but I aspire to a higher level of rspecing

Comment on lines 27 to 35
- name: Runs code QA and tests
env:
RAILS_VERSION: ~> ${{ matrix.rails }}
COVERAGE: no
run: |
sudo apt-get update
sudo apt-get install openjdk-8-jdk
bundle
bundle exec rake rspec

Choose a reason for hiding this comment

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

Suggested change
- name: Runs code QA and tests
env:
RAILS_VERSION: ~> ${{ matrix.rails }}
COVERAGE: no
run: |
sudo apt-get update
sudo apt-get install openjdk-8-jdk
bundle
bundle exec rake rspec
- run: |
bundle exec rake rspec
env:
RAILS_ENV: test

can we simplify this to the above? do we need to install openjdk-8-jdk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think technically no, but will need it if bringing the integration suite in (though it may just become a transitive dep when bringing it cassa)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think RAILS_ENV matters, ruby != rails bro

Co-authored-by: Tate Thurston <tatethurston@gmail.com>
rortian and others added 6 commits July 2, 2024 14:17
Co-authored-by: Tate Thurston <tatethurston@gmail.com>
Co-authored-by: Tate Thurston <tatethurston@gmail.com>
Co-authored-by: Tate Thurston <tatethurston@gmail.com>
Copy link
Collaborator

@tatthurs tatthurs left a comment

Choose a reason for hiding this comment

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

Could you update the allowed push host to only be our package repo in your PR?

Example: https://github.com/art19/cequel/blob/master/cequel.gemspec#L6

@rortian rortian merged commit a134e1e into master Jul 5, 2024
3 checks passed
@rortian rortian deleted the jlcorbet/dep-fun branch July 5, 2024 16:20
rortian added a commit that referenced this pull request Jul 10, 2024
rortian added a commit that referenced this pull request Jul 10, 2024
rortian added a commit that referenced this pull request Jul 10, 2024
* Revert "Updates for ruby 3.0 (#1)"

This reverts commit a134e1e.

* Revert "Fix a case sensitivity issue when fetching the clustering order"

This reverts commit 71e70bf.

* Revert "Update docs.yaml to reflect recent work on docs"

This reverts commit 5f41fa9.

* Revert "changed 4.0+ to 4.0.x"

This reverts commit aa6e17d.

* Revert "readme and docs.yaml updates"

This reverts commit 28cc9aa.

* Revert "DOC-4040 update to docs.yaml (datastax#274)"

This reverts commit c18f98e.

* Revert "Add varchar to Cassandra::Types.text mapping (datastax#270)"

This reverts commit 2c9070c.

* Revert "Slight refactorig of prior commit.  Main goal was to preserve ordering of hosts on the"

This reverts commit 022906d.

* Revert "Merge pull request datastax#267 from abicky/fix-replication-strategies-simple"

This reverts commit 42c435d.

* Revert "Minor spec fixes"

This reverts commit 765f9c9.

* Revert "Merge pull request datastax#265 from EasyPost:log_in_protocol"

This reverts commit e1b8dcb.

* Revert "Quick testing fix; make check for enable_materialized_views YAML config more"

This reverts commit 8fdcfd2.

* Revert "RUBY-331: Migrate to Jenkins pipelines (datastax#264)"

This reverts commit 572dec0.

* Revert "Revert "Fix a case sensitivity issue when fetching the clustering order""

This reverts commit 23ce3b9.

* Revert "Revert "Updates for ruby 3.0 (#1)""

This reverts commit 1b3f6ea.

* Version bump to 3.2.5.2
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.

4 participants