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

Release 2.0 #207

Draft
wants to merge 55 commits into
base: master
Choose a base branch
from
Draft

Release 2.0 #207

wants to merge 55 commits into from

Conversation

samer-hamood
Copy link
Contributor

@samer-hamood samer-hamood commented Jan 22, 2025

This PR includes multiple improvements and a new function, with others included in separate PRs linked below (for ease of review). There will be a small change in the API, referenced in the CHANGELOG.md, which warrants the release to increment its major version to 2.

The core of this PR is:

  • Renamed head_option and last_option to head_not_none and last_not_none respectively,
  • Added first_not_none to match head_not_none
  • Added parametrize for parameterized tests
  • Fixed tests not running from anywhere in the project or via Pycharm (or other IDEs)
  • Added run-test.sh for running checks locally
  • Added support for Python 3.12 and 3.13 by using ruff instead of Pylint due to issues for those Python versions
  • Upgraded pre-commit-hooks and ruff-pre-commit repo revisions
  • Reference to repurposing of two functions, head_option/first_option and last_option, in CHANGELOG.md
  • Improved language, correcting any consistency, grammar and punctuation in some of the docs
  • Linked references readme
  • Upgraded Poetry to 2.0.0
  • Replaced deprecated tool.poetry.dev-dependencies
  • Created partial function in ParallelExecutionEngine only once per instance and added symmetrical code from it to ExecutionEngine (minor refactoring)
  • Fixed linter errors/warnings

Functions this release

Boolean functions

  • any and all taking functions to be equivalent to exists and for_all respectively
  • none - return true if no element satisfies the given predicate function or all elements are falsy if no function is supplied

Indexed functions

Not-none functions

…or release due to breaking change with redefinition of head_option and last_option functions
and correct some spelling, grammar and punctuation
…be removed in a future version, replacing it with "poetry.group.dev.dependencies" instead
@samer-hamood samer-hamood marked this pull request as draft January 22, 2025 22:06
@samer-hamood samer-hamood marked this pull request as ready for review January 22, 2025 23:35
if: matrix.python-version != '3.12' && matrix.python-version != '3.13'
- name: Ruff (Python v${{ matrix.python-version }})
run: poetry run ruff check functional
if: matrix.python-version == '3.12' || matrix.python-version == '3.13'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pylint seems to have issues with Python versions 3.12 and 3.13 - can add another PR for Ruff to more closely match Pylint's rules

@EntilZha
Copy link
Owner

Hi, thanks for the PR!

The main thing I'll mention is that I definitely have a strong preference for 1 Change -> 1 PR instead of many changes -> 1 big PR. Could you break things down?

As with prior PRs, for changes that make not functional changes and are strict improvements, I tend to merge those pretty quickly. Examples in your PR probably include things like improving tests, CI/tooling, fixing lint errors, docs, etc.

This makes reviewing code easier/faster, makes landing "easy" changes faster, and get more changes in, in the case I don't want a particular part of a huge PR. In this specific case as well, should there be a need to have a 2.0, it will also allow all the changes not needed for that land first and thus be available <2.0.

For functional changes, I usually like to see a good explanation for why they are needed. For example, the renames for head_option and last_option seem unnecessary, but I'm open to discussion. I can see the motivation for the name, but the naming comes from the more general idea of option types and isn't python specific (and one original motivation for the names is consistency with scala/similar libraries with those kinds of semantics). This is another reason to break the big PR down, even if I don't want to change the name, its not blocking any of the other changes that are ready to merge.

For new functions, these should also be in their own self contained PR, along with at least some discussion of why they are needed, particularly given the ability to register custom functions.

Again thanks for the PR, it would just help me a lot in getting changes in if PRs are broken into 1 semantic change -> 1 PR.

@samer-hamood
Copy link
Contributor Author

I'll break up the PR.

For new functions, these should also be in their own self contained PR, along with at least some discussion of why they are needed, particularly given the ability to register custom functions.

Could you point to where that (register custom functions) is in the docs or explain how that's done?

For functional changes, I usually like to see a good explanation for why they are needed. For example, the renames for head_option and last_option seem unnecessary, but I'm open to discussion. I can see the motivation for the name, but the naming comes from the more general idea of option types and isn't python specific (and one original motivation for the names is consistency with scala/similar libraries with those kinds of semantics). This is another reason to break the big PR down, even if I don't want to change the name, its not blocking any of the other changes that are ready to merge.

I'll describe the reason for the name change in a separate PR.

@EntilZha
Copy link
Owner

@samer-hamood I misremembered and its called extend, which was merged here https://github.com/EntilZha/PyFunctional/pull/144/files

FWIW, the feature isn't documented outside of the docstring, so that would be a pretty easy improvement to make. My general thinking around adding functions is that if its something that would have somewhat general use, it might be worth adding to the library, but if its relatively niche, then extend is a good way to go.

@samer-hamood samer-hamood marked this pull request as draft January 23, 2025 20:58
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.

2 participants