All committers have signed the CLA.

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

• @enavarro51
• @Qiskit/terra-core

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

• @enavarro51
• @Qiskit/terra-core

Currently, tests are not passing due to a circular import, most likely due to using _is_valid_justify_arg, in all the files...

I had a couple of quick ideas, but it requires moving this function definition, to places, where it doesn't feel right..

Update: Regex check has been removed, for the sake of not complicating this more! 👍

I'm getting this error in the Preliminary tests now, due to the release note:

lint: commands[4]> pylint -rn qiskit test tools

------------------------------------
Your code has been rated at 10.00/10

lint: commands[5]> python /home/vsts/work/1/s/tools/verify_headers.py qiskit test tools examples crates
lint: commands[6]> python /home/vsts/work/1/s/tools/find_optional_imports.py
lint: commands[7]> python /home/vsts/work/1/s/tools/find_stray_release_notes.py
lint: commands[8]> reno -q lint
  docs: FAIL code 1 (606.14=setup[410.84]+cmd[24.06,171.24] seconds)
  lint: OK (673.33=setup[175.87]+cmd[39.08,2.25,0.73,53.68,397.27,0.16,0.66,0.08,3.56] seconds)
  evaluation failed :( (1279.88 seconds)

##[error]Bash exited with code '255'.

In local everything passes correctly, but I think its not checking new files...

Also, I have tried various changes I thought would solve it, but nothing worked so far.
If you have encountered this before, any help would be appreciated.

@GuillermoAbadLopez reno lint was a red herring :/ The warning is from Sphinx higher up in CI logs:

/home/vsts/work/1/s/docs/release_notes.rst:438: WARNING: Inline interpreted text or phrase reference start-string without end-string.

--

I talked more to @1ucian0 and @kevinsung we decided to indeed go back to using None as the default argument. While it makes the documentation a little less clear, Qiskit SDK tends to use None as the default like this and then set the "true default" at runtime because it gives more flexibility for the future.

For example, imagine we want to change the default of left to a new value like justified -- that is a breaking behavior change and would need a deprecation cycle. To detect when people are using the default—rather than explicitly setting ='left'—we would need to check if arg is None. But we can't do that check anymore if we've now set the default directly to be 'left'; we no longer know if you explicitly set the value or you're relying on the default.

So, please go back to using None as the default arg, which results in left being used. But, of course, still warn on otherwise invalid arguments.

Pardon the back-and-forth and thank you for persistence on this issue!

@GuillermoAbadLopez reno lint was a red herring :/ The warning is from Sphinx higher up in CI logs:

/home/vsts/work/1/s/docs/release_notes.rst:438: WARNING: Inline interpreted text or phrase reference start-string without end-string.

That makes a lot of sense, found something, thanks! 👍

No worries at all, we should do what its best for the stack, in the end!

I'll change it, but it might take a bit, since I'm going OOO for a couple of days 👍

I talked more to @1ucian0 and @kevinsung we decided to indeed go back to using None as the default argument. While it makes the documentation a little less clear, Qiskit SDK tends to use None as the default like this and then set the "true default" at runtime because it gives more flexibility for the future.
...

Pull Request Test Coverage Report for Build 9584452045

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 18 of 18 (100.0%) changed or added relevant lines in 3 files are covered.
• 22 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 89.753%

Totals
Change from base Build 9578841868:	-0.02%
Covered Lines:	63571
Relevant Lines:	70829

💛 - Coveralls

Last suggestions addressed!

1. Have gone back to None as the default value for justify

2. Simplified the deprecation logic to a simple warn, similar to the starting one.
   (Since even though the @arg_dreprecation decorator/wrapper was better for localizing the error, it seemed a bit confusing for the future maintainer that would have finally deprecated this, with single lines around to delete, that he could have missed..., now it's super simple! Also this way allows for a Regex check in the test)

3. Some final changes to the docstrings (both public and private), and updated the release note.

@Eric-Arellano @kevinsung @1ucian0

Pull Request Test Coverage Report for Build 9741924018

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 18 of 18 (100.0%) changed or added relevant lines in 2 files are covered.
• 13 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.006%) to 89.767%

Totals
Change from base Build 9718390567:	-0.006%
Covered Lines:	64079
Relevant Lines:	71384

💛 - Coveralls