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

Modification of the CLI to include the use of ElabFTW methods. #75

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

killianrochet
Copy link
Collaborator

Hello @JuliaSprenger ,

I started to modify the CLI, trying to follow the same method as for redcap. I'd like some feedback on the start of the modifications.

Thank you!

The methods / options will be modified as the methods are developed

The methods / options will be modified as the methods are developed
@JuliaSprenger JuliaSprenger added the safe to test Required label for GH action to run testsuite label May 22, 2023
Removed the addition of a new parser.

Now only one option added to define if we want to use the RedCap server or the ElabFTW server
Removed the addition of a new parser.

Now only one option added to define if we want to use the RedCap server or the ElabFTW server
@killianrochet
Copy link
Collaborator Author

Yes, for the tests I have to modify them as well

@JuliaSprenger
Copy link
Collaborator

You should not need to modify the cli-redcap tests, but only extend them by elab related tests.

Modification of the test function for the CLI
Modification of the test function for the CLI
Modification of the test function for the CLI
redcap_bridge/cli.py Outdated Show resolved Hide resolved
Modification of the CLI tests
@killianrochet
Copy link
Collaborator Author

@JuliaSprenger I have an error in the tests. Is it because I created an experiment_id argument? I think the error comes from the fact that it is expected even if I use the redcap server, no? If that's the case, I should only add this argument when I use the elabftw server, right?

redcap_bridge/cli.py Outdated Show resolved Hide resolved
redcap_bridge/cli.py Outdated Show resolved Hide resolved
@killianrochet
Copy link
Collaborator Author

@JuliaSprenger Do you see something that need to be change ? For the output, as I said, it's a first basic version, i think i'll update the CLI after merging the branch containing the download function

@@ -50,3 +50,10 @@ def test_download(initialize_test_dir):
stdout=subprocess.PIPE)
assert 'error' not in str(result.stdout)
assert pathlib.Path(output_file).exists()

# download with Elabftw server
result = subprocess.run(['RedCapBridge', 'download', SERVER_CONFIG_YAML, '--server', 'elabftw', '232'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you are assuming the content of experiment id 232 on the server. However this won't work for another elab server or when the server is cleaned or someone manually deletes that experiment. It would be better to upload a known experiment first to then test the download function on that new experiment. Ideally you would also remove that experiment again from the server afterwards to keep everything in a clean state.

@JuliaSprenger
Copy link
Collaborator

Hi @killianrochet With the big refactoring recently this PR is a bit outdated and some of the issues you address here can be solved in a more elegant way now using the new elab server functionalities. Could you bring the changes up to date? Looking at the few lines that have been changed here and it might be the easiest to start a new branch from the current master instead of rebasing / merge this outdated branch...

@JuliaSprenger JuliaSprenger added this to the version 0.0.3 milestone Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Required label for GH action to run testsuite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants