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

6 0 dev #731

Closed
wants to merge 31 commits into from
Closed

6 0 dev #731

wants to merge 31 commits into from

Conversation

nemesit
Copy link

@nemesit nemesit commented Feb 25, 2020

No description provided.

@nemesit
Copy link
Author

nemesit commented Feb 25, 2020

down to 45 errors 55 fails in active record tests and 0 fails 0 errors in sqladapter

@dglaser
Copy link

dglaser commented Feb 25, 2020

May want to NOT include the testing - b4f558e commit. It assigns @sqlserver_options to an empty hash, likely causing other issues. @ttilberg discussed this HERE

He provided a patch to my PR that changed the altered code back.

@ttilberg
Copy link

@dglaser I was just reviewing these changes, and it looks like a future commit does indeed fix that issue. At the end of this changeset, it includes assignment with **sqlserver_options.

This looks like great progress!

@dglaser
Copy link

dglaser commented Feb 25, 2020

  • The Gemfile needs to be reverted back to remove hard coded rails and tiny_tds gems.
  • The VERSION file represents what version we have the adapter at. Can use
    export RAILS_VERSION='6.0.2.1' to choose the rails version.
  • I'm not certain we should change the Docker image in Dockerfile & docker-compose. Think its best to stay with a standard image that is kept updated with this project, as @wpolicarpo has done so far.

@nemesit
Copy link
Author

nemesit commented Feb 26, 2020

could do yeah, though official tinytds also needs to be updated a bit for ruby 2.7, and it results in one less error too. The rest sure, just wanted up to date images to test

@dglaser
Copy link

dglaser commented Feb 26, 2020

I was only throwing in my 2 cents. As a user, I appreciate wanting an up-to-date image. Maybe the source could be a git repo here for us to submit PR's to and the "owners" to deploy to rails-sqlserver hub??

wpolicarpo and others added 6 commits February 26, 2020 14:25

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…nitest-calls

Fix minitest expectation deprecation warnings

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…n-tests

Calculate should not remove ordering for MSSQL

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…tinct-select-exists

Order by selected items when using distinct exists
wpolicarpo and others added 7 commits February 28, 2020 11:38

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ult-precision

Use default precision for 'time' column type

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…nt-cache-tests

Adapter does not use prepared statement cache

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ult-precision-refactor

Set default time precision when registering time type

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ns-tests

Quoted table names containing square brackets need to be regex escaped
@aidanharan
Copy link
Contributor

The tests on master are now passing and the changes have been pulled into the 6-0-dev branch.

We should get the adapter working against Rails 6.0.0 before moving onto higher releases so that older releases can be supported.

@nemesit: Would you be able to merge the latest changes into your nemesit:6-0-dev branch?

…erver-adapter into 6-0-dev

* 'master' of github.com:rails-sqlserver/activerecord-sqlserver-adapter:
  Quoted table names containing square brackets need to be regex escaped
  Adapter does not use prepared statement cache
  Set default time precision when registering time type
  Default precision for 'time' column type is 7
  Order by selected items when using distinct exists
  Calculate should not remove ordering for MSSQL
  Fix minitest expection calls
  Prepare for Minitest6
  Use rails backtrace in tests

# Conflicts:
#	test/cases/column_test_sqlserver.rb
#	test/cases/rake_test_sqlserver.rb
#	test/cases/showplan_test_sqlserver.rb
#	test/cases/specific_schema_test_sqlserver.rb
#	test/cases/transaction_test_sqlserver.rb
@nemesit
Copy link
Author

nemesit commented Mar 26, 2020

done

@aidanharan
Copy link
Contributor

Maybe its a rebase that needed as there are still conflicting files and on the 'Files Changes' tab you see changes that are already in the activerecord-sqlserver-adapter/6-0-dev branch.

In general, I think it would be better if the fixes in this PR were broken into separate PRs describing what they are fixing. This PR contains a mixture of bug fixes, docker image changes, Gemfile changes, etc. It is better to have PRs where just one issue is just being discussed. I just did that with #748

@wpolicarpo
Copy link
Member

@nemesit do you mind rebasing your branch? I have pushed new Docker images for all newer rubies now and moved them to a new railssqlserver org (they were under my username before).

@wpolicarpo wpolicarpo self-requested a review April 24, 2020 16:27
@wpolicarpo
Copy link
Member

Closing this in favor of #690.

@wpolicarpo wpolicarpo closed this May 5, 2020
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

9 participants