-
Notifications
You must be signed in to change notification settings - Fork 183
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
REL: Prepare for 0.6.4
#1424
REL: Prepare for 0.6.4
#1424
Conversation
I'm open to removing |
sounds good to me, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few questions.
asv/_rangemedian.cpp
Outdated
.tp_doc = NULL, | ||
.tp_methods=RangeMedian_methods, | ||
.tp_init=(initproc)RangeMedian_init, | ||
.tp_new=RangeMedian_new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over at cython, using this in c++ requires c20, so it errored out on windows with error C7555: use of designated initializers requires at least '/std:c++20'
. Does it work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Designated initializers is a C++20 feature but also C99 feature, so here this should be fine (this is also now the recommended style in the Python documentation). Although in the rewrite it would be much much better to cleanly separate the C++ parts (vector
etc) from the Python bindings C
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually thinking about this a bit more I'm just going to revert the change here, there's a lot of weirdness here including the use of a template in a .cpp
:'D
# if `name: artifact` is omitted, the action will create extra parent dir | ||
name: artifact | ||
path: dist | ||
- uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you doing this differently now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never set up trusted publishing so this wasn't running, and shouldn't have been here. The release notes clarify this, explaining that we use twine
for manual management.
I prefer manual management since you can test the wheels before uploading, the trusted publisher workflow means the only way to fix a bug is either a new release or a yank.
This is a bugfix release, so not much else, I think the steps here are what I'll need to follow post-merge. @lucascolley let me know if this seems like a reasonable amount of detail.