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

Python implementation #113

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Python implementation #113

wants to merge 12 commits into from

Conversation

jsa34
Copy link

@jsa34 jsa34 commented Sep 16, 2024

🤔 What's changed?

Added python implementation of kit for publishing to PyPI

⚡️ What's your motivation?

Able to test upcoming messages.

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@jsa34 jsa34 changed the title Python impl Python implementation Sep 16, 2024
@jsa34
Copy link
Author

jsa34 commented Sep 16, 2024

A second pair of eyes would be great for the publishing as I haven't tried to publish a directory outside the module before ("features" directory)

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Cheers. Mostly looks good.

But it does look like you've copied the ruby implementation. This includes quite a few files that aren't essential to the CCK (we are in the process of cleaning them up). It would be better to copy the javascript implementation.

Ideally we only distribute the feature files (input) and the ndjson files (output).

python/pyproject.toml Show resolved Hide resolved
python/src/cck/examples.py Show resolved Hide resolved
python/tests/test_files.py Show resolved Hide resolved
Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Some additional minor thoughts

.github/workflows/release-pypi.yaml Outdated Show resolved Hide resolved
python/pyproject.toml Outdated Show resolved Hide resolved
python/src/cck/examples.py Show resolved Hide resolved
python/tests/test_files.py Show resolved Hide resolved
@mpkorstanje mpkorstanje self-requested a review October 2, 2024 10:46
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Looks good now.

@jsa34 I've somewhat simplified the release scripts. And made them look more like Ruby. Can you check I didn't do anything silly?

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

The example situation needs fixing to replicate a legitimate consumption of the CCK.

Also you might need to extend the docs to support the new 'empty' feature which was merged in recently. This one will likely slip through silently because it's the one item that doesn't need step definitions writing as there are none!

- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Last minute check: Do we want to run the release scripts in 3.8 - @jsa34 just confirm this if you can

Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self - re-reviewing the whole PR as I'd forgotten about it, this comment still stands

- uses: actions/checkout@v4
- uses: actions/setup-python@v4
with:
python-version: '3.8'
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What version support do you want for the CCK. Ideally this should be a lower-bounded version of whatever the consuming cucumber flavour supports (i.e. for the CCK we support a similar amount of versions to the equivalent cucumber-ruby flavour)


2. Set up your test environment. Here's an example of how you might set up a test using `pytest`:

```python
Copy link
Contributor

Choose a reason for hiding this comment

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

Important TOFIX: This example / documentation should be added to be something functionally equivalent to "HOW" your cck will run in the consuming cucumber flavour.

The sample here is the internal tests for the library which wouldn't be copied by a consuming cucumber. For cucumber ruby the documentation example is here: https://github.com/cucumber/compatibility-kit/blob/main/ruby/README.md#installation-and-usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder: This will need fixing. See the link above

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Make sure to make the requisite changes in the devkit folder too to ensure that the script will generate your files.

Running locally inside the devkit folder and then running the npm script should locally generate all files that will then populate your CCK package proper

- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self - re-reviewing the whole PR as I'd forgotten about it, this comment still stands

@@ -0,0 +1,36 @@
name: Release Python Package
Copy link
Contributor

Choose a reason for hiding this comment

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

I've standardised all the workflows so far I'm working on just to say "Release X" where X is the name of the language. The filename can stay the same.

This way in large polyglots I know exactly what's being released


jobs:
test-python:
defaults:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this is correct, recently we reverted. Can you add the npm script inside this entire job (Will need adding).


2. Set up your test environment. Here's an example of how you might set up a test using `pytest`:

```python
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder: This will need fixing. See the link above

pytest = "^7.0"

[build-system]
requires = ["poetry-core>=1.0.0"]
Copy link
Contributor

Choose a reason for hiding this comment

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

note to check - we've had issues before with specific poetry versions, can we ensure this will work on old and new versions of python so it's not a barrier

'cdata',
'data-tables',
'examples-tables',
'hooks',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add empty in here also

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