search

Frequency-Tagging / freq-tag

A Python package facilitating the analysis of frequency-tagging data
Other
3 stars 0 forks source link

set up the template #2

Closed kalenkovich closed 3 years ago

kalenkovich commented 3 years ago

I had to start from scracth. There is still no actual frequency-tagging code added yet. Just making sure everything works, tests are passed, docs are built, etc.

I did pretty much just what is described in the docs for the mne-tools project template here. Not everything worked straight away though - thus a thousand commits. Also, I am not sure what half the CI-like services are doing but I connected them anyway. Appveyor failing is fine, by the way: I haven't told it what to actually do yet.

@dominikwelke, could you please check that you can build the package, the docs and the tests are passed locally? I put the instructions under contributing.md.

Just complaining: The darndest thing is that it took me an extra million years just because open and codecs.open behave a bit differently on Windows. Because of that I always got an error when trying to deploy to PyPI. And the error message was so "informative" and the error itself was so common that I found this image long before I actually figured out how to fix it:

pep8speaks commented 3 years ago

Hello @kalenkovich! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-04-25 14:54:13 UTC
kalenkovich commented 3 years ago

There are some pep8 issues - I'll fix those before merging.

dominikwelke commented 3 years ago

two things though:

dominikwelke commented 3 years ago

Just complaining: The darndest thing is that it took me an extra million years just because open and codecs.open behave a bit differently on Windows. Because of that I always got an error when trying to deploy to PyPI. And the error message was so "informative" and the error itself was so common that I found this image long before I actually figured out how to fix it:

:D

kalenkovich commented 3 years ago
  • [x] setup works
  • [x] coverage test works
  • [x] tests passed
  • [x] doc builds

Thanks for checking!

two things though:

  • i'll make some inline suggestions to the instructions in contributing.md.

A bit confused about that one. You've already committed your suggestions, right?

  • creating the conda environment from environment.yml: i'm not super familiar with it, but if i read the file correctly numpy, sciypy, and mne are fetched twice - once via conda in the environment.yml file, and once via pip in the requirements.txt file. i assume conda will have priority. we could eliminate them from either of the requirement files

conda won't automatically install stuff from requirements.txt so there won't be any double installation. I am not entirely sure but I think some of the CIs might set up the environment using requirements.txt so I'd rather keep it.

dominikwelke commented 3 years ago

A bit confused about that one. You've already committed your suggestions, right?

yes, i was confused too :D my intention was to suggest changes, but then i did a normal commit.. can you remind me how to suggest changes via the github website? ppl in the mne prs do it all the time, but i didnt find it earlier.

conda won't automatically install stuff from requirements.txt so there won't be any double installation. I am not entirely sure but I think some of the CIs might set up the environment using requirements.txt so I'd rather keep it.

alright, my bad.. i was reading to fast - i thought it would pip install the requirements.txt in root as well 

  - pip:
      - -r file:freqtag/tests/requirements.txt
      - -r file:doc/requirements.txt
      - twine
kalenkovich commented 3 years ago

yes, i was confused too :D my intention was to suggest changes, but then i did a normal commit..

Ah. alright. My intentions and my results often diverge as well when it comes to GitHub :) My first ever PR: just merged it myself immediately after creating it. We then decided to change some settings so I wouldn't be able to merge at all :)

can you remind me how to suggest changes via the github website? ppl in the mne prs do it all the time, but i didnt find it earlier.

Sure! You go under the "Files changed" tab. There, you select a line or a couple of lines and press "+" to add a comment. In the comment toolbar above the editing area, there is a button with a plus-minus sign which allows you to add a suggestion alongside the comment.

kalenkovich commented 3 years ago

Since otherwise everything seems to work, I'll just fix the PEP8 issues and merge.