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

Fix C# README examples and add issues refs to recent TODOs #638

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

farost
Copy link
Member

@farost farost commented Apr 22, 2024

Usage and product changes

C# driver's examples should be able to be built in any namespace now.

Some of the recent TODOs left in the codebase have been cleaned or marked with a specific GitHub issue number.

Implementation

We suggest leaving refs to GitHub's issues for each newly merged TODOs not to lose track of these problems and have more context behind them. It can be done:

  • #issuenumber if the issue is a part of the target repo;
  • full URL if the issue is a part of another repo.

@@ -57,7 +57,7 @@ class TestRunner : public TestRunnerBase {
steps.reserve(totalSteps);
for (const StepCollection<CTX> stepVec : stepLists) {
for (const cucumber_bdd::StepDefinition<CTX> stepDef : stepVec) {
steps.push_back(StepDefinition<CTX>{stepDef.regex, stepDef.impl}); // TODO: Avoid deep copy?
steps.push_back(StepDefinition<CTX>{stepDef.regex, stepDef.impl});
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't look like we will be interested in fixing it in the future. @krishnangovindraj approved this TODO's removal.

@@ -293,7 +293,7 @@ build:

source tool/test/start-cloud-servers.sh 3 && # use source to receive export vars
bazel test //python/tests/integration:test_connection --test_env=ROOT_CA=$ROOT_CA --test_output=streamed --jobs=1 &&
# TODO currently broken test
# TODO #635: currently broken test
Copy link
Member Author

Choose a reason for hiding this comment

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

This format comes from the short links format, but we can switch to full URLs if it's not that comfortable. I'm open to discussions.

Copy link
Member

Choose a reason for hiding this comment

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

Short is ok!

@farost farost merged commit 4cf7938 into typedb:development Apr 22, 2024
@farost farost deleted the readme-and-todos-fix branch April 22, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants