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

fs_find_one #150

Open
wants to merge 7 commits into
base: client
Choose a base branch
from
Open

fs_find_one #150

wants to merge 7 commits into from

Conversation

sl5035
Copy link

@sl5035 sl5035 commented Mar 6, 2023

function to find one document from the database using filesystem

Copy link
Collaborator

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this looks good, but let's discuss a bit what we what the API interace to look like. what do we want to hand to find_one and what do we want it to return?

Is this how it is done in regolith, passing in strings? Or do we pass in a dabase object?
Suggest if you think it is not a great idea, but I think stealing everything from regolith, which we know works quite well, is a good idea. It means that to implement your test we may need to bit the bullet and grab runcontrol.py from regolith and the rest of the rc setup. If there is another model that is not regolith, but a more or less standard python CRUD setup, we can steal that too.


**Changed:**

* find_one function for filesystem
Copy link
Collaborator

Choose a reason for hiding this comment

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

We just need here under Added: find_one function for fs client.

The way to think about "news" is it is for users of the software to see what changed since the last release (this gets built into the changelog) so it serves a different purpose than the commit messages...no need to mention there is a test for the function you are building.

Copy link
Author

Choose a reason for hiding this comment

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

Ah makes sense! Thank you

def test_find_one():
pass
client = FileSystemClient(rc)
cif_path1 = "../docs/examples/cifs/calculated/bs0018IIIsup4.rtv.simulated.cif"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is brittle and bound to break. copy-paste the content into a variable in a python module and import it. Call the module something like testing_cif_inputs. Then use TempDirectory to write them to a tempfile.

@sl5035
Copy link
Author

sl5035 commented Mar 7, 2023

  • "pre-commit hook" commit should be "minor bug fix for tempdirectory and news"
  • Implementing runcontrol seems like a good idea for now, I will make another branch for it and will be working on it instead

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.

2 participants