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

Open dataset #14

Merged
merged 13 commits into from
Jul 22, 2021
Merged

Open dataset #14

merged 13 commits into from
Jul 22, 2021

Conversation

NicolasLegendre1
Copy link
Contributor

No description provided.

Copy link
Member

@fredericpoitevin fredericpoitevin left a comment

Choose a reason for hiding this comment

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

Pending @ninamiolane approval and successful automated checks, it looks like all my concerns were addressed; great job @NicolasLegendre1 ! Thanks for your hard work

iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Looks good to me as soon as my very minor comments are addressed. Great work @NicolasLegendre1 , thank you for your patience there!!

As a global comment, please sort the imports in alphabetic order in each file (this is supposed to be done automatically, but for some reason our "transformer" doesn't work).

The only puzzling thing is that DeepSource transformers are not triggered on this PR? (see the GitHub actions checks below?) @NicolasLegendre1 @fredericpoitevin I wonder if Nicolas you have to enable something on your side, or if it is on our side Fred.

iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
iospi/iotools/cryo_dataset.py Show resolved Hide resolved
iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
iospi/iotools/cryo_dataset.py Outdated Show resolved Hide resolved
tests/test_iotools/test_cryo_dataset.py Show resolved Hide resolved
tests/test_iotools/test_cryo_dataset.py Outdated Show resolved Hide resolved
@ninamiolane
Copy link
Contributor

@fredericpoitevin could you see if you can ignore the following deepsource errors on our compSPI repo?

image
image

We don't want to enforce docstrings in test functions, I think this will be too much for the contributors. Then, the error on the assert is weird, as we are indeed in test files.

I know that it is possible to ignore specific errors in specific modules.py with deepsource, but for some reason I cannot access the deepsource page of compSPI, maybe I don't have the credentials? It should be under the Settings tabs, once we log in to deepsource and go to the https://deepsource.io/gh/compSPI/ioSPI/ page...

@fredericpoitevin
Copy link
Member

Looks good to me as soon as my very minor comments are addressed. Great work @NicolasLegendre1 , thank you for your patience there!!

As a global comment, please sort the imports in alphabetic order in each file (this is supposed to be done automatically, but for some reason our "transformer" doesn't work).

The only puzzling thing is that DeepSource transformers are not triggered on this PR? (see the GitHub actions checks below?) @NicolasLegendre1 @fredericpoitevin I wonder if Nicolas you have to enable something on your side, or if it is on our side Fred.

I believe it's on our side: I remember seeing some settings about that at some point but can't find it now...

@fredericpoitevin
Copy link
Member

fredericpoitevin commented Jul 20, 2021

@fredericpoitevin could you see if you can ignore the following deepsource errors on our compSPI repo?

image
image

We don't want to enforce docstrings in test functions, I think this will be too much for the contributors. Then, the error on the assert is weird, as we are indeed in test files.

I know that it is possible to ignore specific errors in specific modules.py with deepsource, but for some reason I cannot access the deepsource page of compSPI, maybe I don't have the credentials? It should be under the Settings tabs, once we log in to deepsource and go to the https://deepsource.io/gh/compSPI/ioSPI/ page...

Good idea about docstrings in test functions. I just tried to set deepsource so it ignores this rule for test file, but the web app seems buggy... I have invited you to the deepsource compSPI team (sorry I did not realize!), you should have received an invite in your gmailbox: let me know if I should send to another email.

Regarding the "assert" issues, I just realized that the last merge changed the test/ directory to tests/ and .deepsource.toml was not updated to reflect that change. I'll try now, this should fix the issue.

UPDATE: @NicolasLegendre1, if you can merge the PR I just opened in your repo NicolasLegendre1#1 and then update this one, it'd be great! I believe it will help with the deepsource issues.

@ninamiolane
Copy link
Contributor

@fredericpoitevin thank you for adding me! I can indeed see the repos on deepsource now. I also see your ignore rules: thank you for adding them.

I tried to directly add ignore rules from this PR (see screenshot), but unfortunately the app is buggy indeed and I can't actually click on it... :(

I will write to DeepSource maintainers about this.

ignorerules

@fredericpoitevin
Copy link
Member

@fredericpoitevin thank you for adding me! I can indeed see the repos on deepsource now. I also see your ignore rules: thank you for adding them.

I tried to directly add ignore rules from this PR (see screenshot), but unfortunately the app is buggy indeed and I can't actually click on it... :(

I will write to DeepSource maintainers about this.

Ah thanks, loop me in if you think I can help!
I suspect some latency issue: yesterday the rule I ignored did not show up as being ignored right away (hence having the same twice today).
@NicolasLegendre1 let me know if you want to chat, I think we can merge this PR very soon 😃

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

@fredericpoitevin Deepsource solved the bug and I was able to ignore the issue. DeepSource is passing now 🎉

@NicolasLegendre1 we're almost there!!

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Let's gooooo!!

@ninamiolane ninamiolane merged commit f324d3c into compSPI:master Jul 22, 2021
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