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

[MAINT] Support Windows' paths; Update zenodo & contributor count #672

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

notZaki
Copy link
Contributor

@notZaki notZaki commented Feb 3, 2021

Closes None.

Few small changes:

  • Use platform-specific path when comparing output files in integration tests
    • This was causing tests on Windows to fails because it expects paths\like\this instead of /like/that
  • Updated the number of contributors on the readme badge
    • There is a configuration option which might automatically do this
    • Looks like there is a mismatch in the number of total contributors between all-contributors and github's contributor list. I went with number of all-contributors.
  • Added myself to .zenodo.json
    • Should this follow the order in the planned JOSS paper? That might be better for consistency, but that means getting rid of The tedana Community and bit of shuffling
    • There is also info/__credits__ which is out of sync and could be removed to simplify maintenance

The full test suite passed on windows, mac, and ubuntu with Python 3.7-3.9 [log]

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #672 (bede0c2) into main (14be852) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #672   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files          26       26           
  Lines        2030     2030           
=======================================
  Hits         1901     1901           
  Misses        129      129           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14be852...bede0c2. Read the comment docs.

README.md Show resolved Hide resolved
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

Thanks @notZaki !
It's weird that the patch has that one line both added and subtracted with no apparent change, but I'll go ahead and approve.

@jbteves
Copy link
Collaborator

jbteves commented Feb 4, 2021

The reason for the split is that in "all contributors" we add non-code, non-authored contributions. The GitHub contributors will only list people who have committed or co-authored something added to the main branch. So, "all contributors" is the correct listing for the total number of contributors.

I have no strong feelings about the Zenodo ordering.

@emdupre
Copy link
Member

emdupre commented Feb 5, 2021

Thanks again, @notZaki ! 😄

re Windows : Thank you ! 🙏 We should probably add Azure testing....

re configuration : nice find ! I want to add "sort alphabetically," too. I'll see if I can get that changed. Don't worry about it in this PR.

re mismatch : @jbteves 's explanation matches my understanding :)

re: the zenodo : let's keep this as-is. The decision was that the software would always have the tedana community as first-author, but that individual outputs (like papers) could have different authorship orders / lists depending on who took on the writing, experiments, etc.

re __credits__ : good catch ! I'm in favor of removing it.

@emdupre
Copy link
Member

emdupre commented Feb 5, 2021

I'm going to approve and merge this now so it goes into the release (:tada: ) and we can open another issue to remove __credits__ and set the all-contributors bot config.

🚀 🚀 🚀

@emdupre emdupre merged commit 08f6a87 into ME-ICA:main Feb 5, 2021
@notZaki notZaki deleted the windows branch March 19, 2021 17:29
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