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

Add initial infrastructure #12

Merged
merged 12 commits into from
Nov 13, 2020
Merged

Add initial infrastructure #12

merged 12 commits into from
Nov 13, 2020

Conversation

smoia
Copy link
Member

@smoia smoia commented Nov 10, 2020

Closes #15

Proposed Changes

Complete the missing infrastructure used in other physiopy packages, such as:

  • Auto
  • CircleCI
  • Readthedocs (maybe useless, but we can remove it later)
  • Plus some updates in infrastructure

@smoia smoia added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Nov 10, 2020
Copy link

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

The CircleCI part looks good. You're gonna have to make sure that you've got it set up on the website.

I think you're the one who knows about auto best, so no comments on that.

Thank you @smoia !

.circleci/config.yml Outdated Show resolved Hide resolved
@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

Ok, I'm not sure it's working. @eurunuela can you have a look at it? I thought it would have tbh...

Maybe I need to watch again your tutorial 😅

@eurunuela
Copy link

Okay, I see what the problem is. You didn't add the Makefile.

So, the config file is pointing at the Makefile to run some commands; i.e. the pytest commands. You could just copy the phys2bids Makefile.

@eurunuela
Copy link

It's now working as expected.

You see the tests complain about the --skipintegration option. Once we have some tests ready, we will have to prepare the conftest.py and pass the --skipintegration argument as we do with phys2bids.

@eurunuela
Copy link

If you don't want to have the failing tests, you could comment out the tests you do not want to run just yet from line 206 down to the end of the .config.yml file.

@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

CircleCI is complaining about the lack of flake8 though, is it because we don't have tests?

@eurunuela
Copy link

No. That's cause you don't have flake8 on the setup.cfg file. Copy all the flake8 and testing properties from the phys2bids one.

@eurunuela
Copy link

No. That's cause you don't have flake8 on the setup.cfg file. Copy all the flake8 and testing properties from the phys2bids one.

Now I realize I forgot to mention that on the tutorial 🤣

setup.cfg Outdated Show resolved Hide resolved
@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

Ok, now it should work.

All tests are set up, but none is running atm, except for style checks.

Copy link

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

It think it's all ready now!

@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

Any idea on why this is a problem here but not in phys2bids?

@eurunuela
Copy link

Any idea on why this is a problem here but not in phys2bids?

What problem? There are no tests in phys2denoise as of now.

If you're talking about the style check, you will have to tune the flake8 settings to ignore some errors. If that's what you want.

.circleci/config.yml Outdated Show resolved Hide resolved
phys2denoise.py Outdated
Comment on lines 390 to 404
"""
Copyright 2019, The Phys2BIDS community.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious- why is this in the actual file? Doesn't the LICENSE file at the top level apply to all files in the repository?

Copy link
Member Author

Choose a reason for hiding this comment

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

We never understood - that's why we put it at the end of the main workflow in phys2bids!

@eurunuela
Copy link

Okay. The test work as you can see.

I'm trying to figure out why the codecov call is not working. Did you give the codecov bot permissions @smoia ?

@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

You mean the problem of the merge_coverage test failing? Could it be because there is no actual test going on on the code?

@eurunuela
Copy link

You mean the problem of the merge_coverage test failing? Could it be because there is no actual test going on on the code?

That's my second option if the bot has permissions 🤣

@smoia
Copy link
Member Author

smoia commented Nov 11, 2020

Yes, the bot seems to have permission!

@smoia smoia requested a review from tsalo November 13, 2020 08:47
@smoia
Copy link
Member Author

smoia commented Nov 13, 2020

If anyone else can give a general check, we can merge this in asap!

Copy link
Contributor

@62442katieb 62442katieb left a comment

Choose a reason for hiding this comment

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

lgtm

(and by that I mean, it's legit)

@smoia smoia self-assigned this Nov 13, 2020
@smoia
Copy link
Member Author

smoia commented Nov 13, 2020

Ok, I'm going to merge in things for the moment.

@smoia smoia merged commit 4a6b21a into physiopy:master Nov 13, 2020
@smoia smoia deleted the int/bots branch November 13, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Travis config file in repo
4 participants