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

String Loader #326

Merged
merged 1 commit into from Sep 21, 2023
Merged

String Loader #326

merged 1 commit into from Sep 21, 2023

Conversation

lucasrcezimbra
Copy link
Contributor

Adds the ability to load pipelines from a string instead of a file path. Useful for testing or for invoking the runner using a pipeline that doesn't come from the file system.

Copy link
Member

@yaythomas yaythomas left a comment

Choose a reason for hiding this comment

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

great idea, and great execution! thank you so much!

it so happens you actually did more work than you needed to with the extra cache... So it's only really that and a minor comment about a comment and then we can move get it merged!

Looking forward to getting this approved and released! Please let me know if you have any questions.

Friendly reminder, when you prepare the next changeset, please remember to squash your commits into one.

pypyr/loaders/string.py Outdated Show resolved Hide resolved
pypyr/loaders/string.py Show resolved Hide resolved
Copy link
Member

@yaythomas yaythomas left a comment

Choose a reason for hiding this comment

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

Nice, this is looking great, thank you!

We just have to get all the build CI checks to pass, which are currently failing on some linting:

--> flake8 linting
./pypyr/loaders/string.py:25:80: E501 line too long (88 > 79 characters)
./tests/integration/pypyr/loaders/__init__.py:1:1: D104 Missing docstring in public package
./tests/integration/pypyr/loaders/string_loader_test.py:1:1: D100 Missing docstring in public module
./tests/integration/pypyr/loaders/string_loader_test.py:7:1: D103 Missing docstring in public function
./tests/integration/pypyr/loaders/string_loader_test.py:24:1: D103 Missing docstring in public function
./tests/integration/pypyr/loaders/string_loader_test.py:35:80: E501 line too long (87 > 79 characters)
./tests/unit/pypyr/loaders/string_loader_test.py:1:1: D100 Missing docstring in public module
./tests/unit/pypyr/loaders/string_loader_test.py:8:1: D103 Missing docstring in public function
./tests/unit/pypyr/loaders/string_loader_test.py:17:80: E501 line too long (83 > 79 characters)

These are just truly minor formatting issues, but the linter keeps everything in the entire code base consistent so it's important :-) If you want to double-check locally that you got all of them, here is how in the documentation - shortly put, run tox ops/build package.

A little tip: if you want to squash your commits easily when you're ready for the final merge, you can do this:

git reset --soft  bc1a53043700738b1269b2f8a5ff019ec818a676
git add .
git commit -m "add String loader"
git push -f

The first line will put the branch pack to the state it was in when it was freshly made from main (that long number is the commit hash at the HEAD of main currently), but leave all your changed files in the working directory so you can re-combine them in a single commit. This is what the remaining steps are doing.

This will leave you with one single commit with all your changes in it, so we have a nice clean commit history and you get sole author credit also for your great work here :-)

@lucasrcezimbra
Copy link
Contributor Author

These are just truly minor formatting issues, but the linter keeps everything in the entire code base consistent so it's important :-) If you want to double-check locally that you got all of them, here is how in the documentation - shortly put, run tox ops/build package.

I didn't read the developer guide before 😁. Thanks.

A little tip: if you want to squash your commits easily when you're ready for the final merge, you can do this:

I sent in many commits to help with the review. Usually, I use Squash and Merge to merge the PR on GitHub.

FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_mapping - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'
FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_sequence - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'
FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_scalar - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'
FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_mapping_substitutions - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'
FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_sequence_substitutions - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'
FAILED tests/unit/pypyr/dsl_test.py::test_jsonify_roundtrip_scalar_substitutions - ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag '!jsonify'

Some tests are failing locally here. I don't know if it's my setup or any bug. I didn't go deep to understand the issue. Let's see if it works on the CI here.

@yaythomas yaythomas merged commit 81e0dba into pypyr:main Sep 21, 2023
0 of 6 checks passed
@yaythomas
Copy link
Member

Thanks for your patience @lucasrcezimbra! The failing tests were because of a breaking change in the yaml parser library dependency, and it had nothing to do with you - sorry for the inconvenience! I fixed that issue and pulled in your excellent feature!

I need to update some documentation and prepare the release notes, and then I'll release so the new version is available on pypi. That should be by this weekend.

@yaythomas
Copy link
Member

release live on pypi - release notes here: https://github.com/pypyr/pypyr/releases/tag/v5.9.0

Still TODO is update the website documentation.

@lucasrcezimbra lucasrcezimbra deleted the string-loader branch September 21, 2023 11:00
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.

None yet

2 participants