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 GitHub Actions workflow 'test' #249

Closed
wants to merge 5 commits into from
Closed

Add GitHub Actions workflow 'test' #249

wants to merge 5 commits into from

Conversation

eine
Copy link

@eine eine commented Jan 31, 2020

This PR is based on #230

Context: #230 (comment)

@tartley
Copy link
Owner

tartley commented Oct 13, 2020

Hey. FYI, Yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it!

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

I think this PR can be deleted if we merge #230

@tartley tartley added the process Changes in our development process, CI, package building, releases, etc label Oct 7, 2021
@eine
Copy link
Author

eine commented Oct 20, 2021

@tartley that is correct. The commits in this PR are included in #230.

@eine
Copy link
Author

eine commented Oct 20, 2021

@tartley I now updated this PR to resolve the conflicts in #230.

Copy link
Owner

@tartley tartley left a comment

Choose a reason for hiding this comment

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

I don't think it makes sense to merge this change until we agree on what a sensible set of github actions would be.

pull_request:
schedule:
- cron: '0 0 * * 5'
workflow_dispatch:
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a fan of running test jobs on a schedule. They should be in response to commits or pushes.

FORCE_COLOR: true
- run: ./demos/demo.sh
env:
NO_COLOR: true
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should be automating the use of 'demo.sh' as a form of test. I'd prefer we leaned more into creating genuine automated tests, not things that need human eyeballs.


- on Windows or if output is redirected (not a tty)
- or if the ``NO_COLOR`` environment variable is set (with any value)
- and ``FORCE_COLOR`` not set (with any value)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these changes should be in this PR, am I misunderstanding?


- on Windows and output is to a tty (terminal)
- or if the ``FORCE_COLOR`` environment variable is set (with any value)
- and ``NO_COLOR`` is not set (with any value)
Copy link
Owner

Choose a reason for hiding this comment

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

don't think these should be here

color_allowed = True
else:
color_allowed = not_closed and self.stream.isatty()

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these changes should be in this PR

@@ -8,6 +8,8 @@
:: Implemented as a bash script which invokes python so that we can test the
:: behaviour on exit, which resets default colors again.

cd /d "%~dp0"

Copy link
Owner

Choose a reason for hiding this comment

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

Help me understand the motivation for these. They are probably sensible, I just like MPs to be self-explanatory, rather than readers speculating what changes are for.

@tartley tartley closed this Jun 14, 2022
@eine eine deleted the ci-gha branch June 15, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Changes in our development process, CI, package building, releases, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants