Skip to content

#4: QueryResultRow does not instantiate the row data #29

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.

Sign up for GitHub

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

Jump to bottom
Merged
benedeki merged 13 commits into from
Aug 26, 2024
Merged

#4: QueryResultRow does not instantiate the row data #29

benedeki merged 13 commits into from
Aug 26, 2024

Conversation

@benedeki
Copy link
Contributor

@benedeki benedeki commented Aug 16, 2024 •
edited
Loading

  • QueryResultRow is now instantiated, so can be used even after connection close
  • columns can be referred by their index not just name (1 based); for that reason QueryResultRow now has columnCount property too
  • QueryResultRow now has rowNumber property too
  • JsonBString deprecated, replaced by SimpleJsonString
  • setup for separation of unit and integration tests
  • integration tests database setup
  • github build action enhanced to support integration tests

Closes #4
Closes #27

benedeki added 3 commits July 17, 2024 15:34
@benedeki
#26: Move code into sub package db
98e7225
@benedeki
#4: QueryResultRow does not instantiate the row data
9f84b12 
* WIP
@benedeki
#4: QueryResultRow does not instantiate the row data
70a5cc0 
* `QueryResultRow` is now instantiated, so can be used even after connection close
* `JsonBString` deprecated, replaced by `SimpleJsonString`
* setup for separation of unit and integration tests
* integration tests database setup
* github build action enhanced to support integration tests
@benedeki benedeki marked this pull request as ready for review August 16, 2024 18:57
@benedeki benedeki requested a review from jakipatryk August 17, 2024 17:48
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
balta/src/test/resources/db/postgres/01_db.ddl
@@ -0,0 +1,6 @@
CREATE DATABASE mag_db
Copy link
Collaborator

@lsulak lsulak Aug 19, 2024

Choose a reason for hiding this comment

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

why mag_ ? :D

Copy link
Contributor Author

@benedeki benedeki Aug 19, 2024

Choose a reason for hiding this comment

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

Well you had to notice, didn't you. 😉
Well, I sense there are parts that might become common to balta, fa-db, ultet (🤭 ) justifying a shared basic lib (mag = seed in Hu)

Copy link
Collaborator

@lsulak lsulak Aug 19, 2024

Choose a reason for hiding this comment

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

curiosity is my superpower :D

Copy link
Collaborator

@lsulak lsulak Aug 19, 2024

Choose a reason for hiding this comment

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

Okay, makes sense!

lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

I finished the review for now, I like it - especially the tests you implemented and integrated them into CI!

benedeki and others added 2 commits August 19, 2024 13:07
@benedeki @lsulak
Apply suggestions from code review
8a01dae 
Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>
@benedeki
* Addressed PR comments
a6237b6
miroslavpojer
miroslavpojer reviewed Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • pull
  • code review

I am missing:

  • code coverage measuring.

Nice work.

.github/workflows/test_filenames_check.yml Outdated

- name: Filename Inspector
id: scan-test-files
uses: AbsaOSS/filename-inspector@master
Copy link
Collaborator

@miroslavpojer miroslavpojer Aug 21, 2024

Choose a reason for hiding this comment

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

Version 0.1.0 has already been released:

lsulak
lsulak previously approved these changes Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Gonna approve from my side now, looks good! However Miroslav had a good point or two, please check his comments before merging

@benedeki
Copy link
Contributor Author

benedeki commented Aug 23, 2024

I am missing:

* code coverage measuring.

This is more a hobby project than an official stream of work, so I don't even dare to target for some coverage. RIght now the test coverage is rather low. So either the limit would be rather low, or the check failing constantly. I don't see the point. 🤷‍♂️

miroslavpojer
miroslavpojer approved these changes Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • pull
  • code review
  • IT test run

@benedeki benedeki merged commit dad52d8 into master Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miroslavpojer miroslavpojer miroslavpojer approved these changes

@lsulak lsulak lsulak left review comments

@salamonpavel salamonpavel Awaiting requested review from salamonpavel salamonpavel is a code owner

@jakipatryk jakipatryk Awaiting requested review from jakipatryk

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add option(s) to get result column without column name QueryResultRow does not instantiate the row data

4 participants

@benedeki @lsulak @miroslavpojer