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

chore(examples): Rename in-memory db in examples. #225

Closed
wants to merge 1 commit into from

Conversation

d-bohls
Copy link
Contributor

@d-bohls d-bohls commented Feb 6, 2018

In our examples, make it clear that in-memory
databases are not restricted to ':memory:'.

Also, it's probably better to avoid using
':memory:' for in-memory databases in case
that might cause conflict with the stream sessions.

  • This contribution adheres to CONTRIBUTING.md.
  • New tests have been created for any new features or regression tests for bugfixes.
  • tox successfully runs, including unit tests and style checks (see CONTRIBUTING.md).

In our examples, make it clear that in-memory
databases are not restricted to ':memory:'.

Also, it's probably better to avoid using
':memory:' for in-memory databases in case
that might cause conflict with the stream sessions.
@d-bohls d-bohls added this to the 0.4 milestone Feb 6, 2018
@d-bohls d-bohls self-assigned this Feb 6, 2018
@d-bohls d-bohls requested a review from jashnani February 6, 2018 17:22
@coveralls
Copy link

coveralls commented Feb 6, 2018

Coverage Status

Coverage remained the same at 67.818% when pulling ea86312 on d-bohls:inMemoryDbExamples into 779b77c on ni:master.

1 similar comment
@coveralls
Copy link

coveralls commented Feb 6, 2018

Coverage Status

Coverage remained the same at 67.818% when pulling ea86312 on d-bohls:inMemoryDbExamples into 779b77c on ni:master.

@@ -12,7 +12,7 @@


def main():
database_name = ':memory:'
database_name = ':CAN_Database:'
Copy link

@jashnani jashnani Feb 7, 2018

Choose a reason for hiding this comment

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

I'd prefer we use :memory: in our examples unless we have two or more in-memory databases being used at the same time. I don't think we should promote using random strings between the colons because there is a convention for this. See database documentation, section Multiple Databases Simultaneously.

"NI-XNET allows opening up to seven distinct databases at the same time. You can open any database from a database file or in memory. To open multiple in-memory databases, use the name :memory[]:; for example, :memory:, :memory1:, :memory2:."

Maybe we can create a new example showing that you can edit/use multiple in-memory databases at the same time. Thoughts?

Copy link
Contributor Author

@d-bohls d-bohls Feb 7, 2018

Choose a reason for hiding this comment

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

I see your point that the documentation/examples seem to encourage the user to use ":memory[n]:" naming for in-memory databases. I'm left wondering if this is really a best-practice or just something like an artifact of brevity in the documentation. I'd like to look into that further.

From my discussion with Jason Smith, I was left with the impression a uniquely named database would be desirable to potentially avoid accidental collisions with another thread that may happen to be using the same database name. For all I know, this situation may be unlikely given how customers use XNET, but it seems possible from what Jason said. I'll follow up on this.

Choose a reason for hiding this comment

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

It probably is best to stick with the naming that is mentioned in the NI-XNET documentation.
The most common use for in-memory databases is going to be stream sessions where the database contents aren't extremely critical. For more advanced uses, I would expect most people to be opening databases from disk.

@d-bohls d-bohls closed this Feb 7, 2018
@d-bohls d-bohls deleted the inMemoryDbExamples branch February 7, 2018 22:58
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.

None yet

4 participants