search

mne-tools / mne-installers

Installers for MNE-Python.
BSD 3-Clause "New" or "Revised" License
9 stars 8 forks source link

MAINT: Raise errors #114

Closed larsoner closed 2 years ago

larsoner commented 2 years ago

It's weird that I had to change the -eq 5 to -eq 3 in https://github.com/mne-tools/mne-installers/pull/113. I want to check to see if it's from the bash -e arg, which errors out immediately on a problem (rather than allowing other commands to continue and override the exit code with something that potentially succeeds).

larsoner commented 2 years ago

@hoechenberger since this has come back green, I propose

  1. We merge this PR, as it "just" ensures that we use -e in BASH and use a programatically determined version. Then
  2. We can isolate any build issues with the _2/post2 conda-forge build in #113 after a rebase/merge with main
hoechenberger commented 2 years ago

@larsoner Feel free to merge if you think this is ready.

Minor request: Could you please not prefix the PR titles with MAINT etc? Since We're auto-compiling the changelog, these prefixes then quickly cause an eyesore. If you insist on adding this kind of metadata, could we agree to use actual tags instead?

larsoner commented 2 years ago

Could you please not prefix the PR titles with MAINT etc? Since We're auto-compiling the changelog, these prefixes then quickly cause an eyesore. If you insist on adding this kind of metadata, could we agree to use actual tags instead?f you insist on adding this kind of metadata, could we agree to use actual tags instead?

I usually just try to follow what most scientific python packages I work on (numpy, scipy, scikit-learn, mne-python, etc.) do / have done for years. The more we make each repo an exception with its own rules, the more rules people need to learn. I'd rather have to tweak the release notes at release time a bit (or live with a minor eyesore) than cause all contributors to have to use and remember a non-standard scheme.

hoechenberger commented 2 years ago

I'd rather have to tweak the release notes at release time a bit (or live with a minor eyesore) than cause all contributors to have to use and remember a non-standard scheme.

Can you not simply leave the prefixes out, then? Nobody really cares about them anyway? And they're not used consistently.

larsoner commented 2 years ago

Can you not simply leave the prefixes out, then?

Of course it's an option / I could. But it's more work for me, and all contributors who adhere to (what seems to be) the current de facto scientific Python standard. So really it's a question of who should have to do the work and/or adapt their practices. I think it makes sense to have contributors follow what we (try to) follow in MNE-Python as much as possible, rather than have this repo be exceptional in some way in terms of how people should contribute. Then if the eyesore really bothers you, we either change how MNE-Python as a whole does things, or you simply adapt the release practices.

Nobody really cares about them anyway?

It's the standard we outline for use in MNE-Python in our contributing guide.

And they're not used consistently.

I would say that's a problem with how people work in MNE-Python really. FWIW if you really want to push this point, I think it makes more sense to discuss at the MNE-Python level than here. You might very well be right that these prefixes aren't helpful anymore, but I'd really rather not make mne-installers an exception to following the "MNE Way".