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

ENH Add $startIndex param to Pos method #1222

Closed
wants to merge 1 commit into from

Conversation

nedmas
Copy link

@nedmas nedmas commented Jul 20, 2024

Description

I've added $startIndex param to the Pos method with a default value of 1.

Manual testing steps

Loop through elements in an elemental area and print the results of $Pos(1).

Issues

Pull request checklist

  • [*] The target branch is correct
  • [*] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • [*] The commit messages follow our commit message guidelines
  • [*] The PR follows our contribution guidelines
  • [*] Code changes follow our coding conventions
  • [*] This change is covered with tests (or tests aren't necessary for this change)
  • [*] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • [*] CI is green

@michalkleiner
Copy link
Contributor

Just curious @nedmas, based on what did you check the CI is green item in the checklist? The CI for this PR hasn't run yet.

Comment on lines -1266 to +1267
public function Pos()
public function Pos($startIndex = 1)
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the initiative, but unfortunately we can't add new parameters (even optional ones) to public API outside of a major release as per the major release policy and definition of public API.

We could accept this in CMS 6, so feel free to submit a new pull request targetting the 6 branch - but I'll close this one.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. It's disappointing that the policy is to class optional parameters as a breaking change. However if that's the policy then fair enough I'll add another PR.

@nedmas
Copy link
Author

nedmas commented Jul 24, 2024

Just curious @nedmas, based on what did you check the CI is green item in the checklist? The CI for this PR hasn't run yet.

So honestly I just marked it complete because I got overwhelmed by the amount of stuff I was required to read/do just to add a minor PR. Tried my best but I guess I got it wrong 😃 Out of interest what should I have done? So I know for next time.

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