Skip to content

Conversation

@ptiurin
Copy link
Contributor

@ptiurin ptiurin commented Mar 15, 2023

With the latest changes to aragog API behaviour has changed. Modifying our tests to account for it.

@ptiurin ptiurin marked this pull request as ready for review March 16, 2023 09:47
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Column(d["name"], parse_type(d["type"]), None, None, None, None, None)
for d in query_data["meta"]
]
if not descriptions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't we just have it empty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, pep 249 says it should be None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it's a bit counter-intuitive to receive empty array in description. For example,
A user does an INSERT query, checks cursor.rowcount is correct then checks for some reason cursor.description and it's []. The name itself implies singular as well.
I don't feel too strongly about it, but this is also how we had it up until now.

Column(d["name"], parse_type(d["type"]), None, None, None, None, None)
for d in query_data["meta"]
]
if not descriptions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, pep 249 says it should be None

@ptiurin ptiurin merged commit 8c2e1f9 into main Mar 17, 2023
@ptiurin ptiurin deleted the test-fix-insert-rowcount branch March 17, 2023 10:59
stepansergeevitch pushed a commit that referenced this pull request Apr 14, 2023
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.

3 participants