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 1.0.0 #1

Conversation

vhrychenko
Copy link
Contributor

@vhrychenko vhrychenko commented Aug 21, 2018

Please confirm

  • No new OS components - or they have been approved by the legal department
  • All new or heavily changed classes get an "A" rating (Scrutinizer), except Facades, Factories, QueryContainers and Tests

Documentation

  • Functional documentation provided or in progress?
  • Integration guide for projects provided or not needed?
  • Migration guides for all contained majors provided or not needed?

Release Table

Module Release Type Constraints
Sevensenders major

Release Notes

  • 1.0.0 version for sevensenders module.

Module Sevensenders

Patch: Backwards-compatible bug fix

Def of done (by the responsible developer):

  • All changes are backwards-compatible. The outdated code is marked as deprecated.
  • The change is isolated and not mixed with any cleanups or other changes.
  • New code fits the architecture rules.
  • All new or changed business logic is covered by functional tests (in Zed business layer).

Change log

  • 1.0 version for Sevensenders module.

@dereuromark
Copy link
Contributor

Please link the release group in the template, as well, for easier access to it.

.scrutinizer.yml Outdated
build:
dependencies:
before:
- composer config repositories.spryker composer https://code.spryker.com/repo/private
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed, and even wrong.
Please use current template.

composer.json Outdated
"spryker/testify": "*"
},
"autoload": {
"psr-0": {
Copy link
Contributor

Choose a reason for hiding this comment

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

psr-4 must be used since end of 2017


/**
* MIT License
* Use of this software requires acceptance of the Evaluation License Agreement. See LICENSE file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using the wrong license. Please check with current specs on .license file for MIT.

public $adapter;

/**
* @var \SprykerEco\Zed\Sevensenders\Dependency\Facade\SevensendersToSalesFacadeBridge
Copy link
Contributor

@dereuromark dereuromark Aug 21, 2018

Choose a reason for hiding this comment

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

Must be the interface.
Please run in level 5 of phpstan, this would find the issues.

* @return bool
*/
public function isTokenValid(SevensendersTokenTransfer $tokenTransfer): bool;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please run code sniffer and setup your IDE properly: The newline missing warnings should not be here.

@vhrychenko vhrychenko changed the title Release 1.0.0 [WIP] Release 1.0.0 Aug 21, 2018

/**
* @method \SprykerEco\Zed\Sevensenders\Business\SevensendersBusinessFactory getFactory()
* @method \SprykerEco\Zed\Sevensenders\Persistence\SevensendersRepositoryInterface getRepository()()
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid () syntax.

@@ -23,7 +23,7 @@
},
"minimum-stability": "dev",
"autoload-dev": {
"psr-0": {
"psr-4": {
Copy link
Contributor

Choose a reason for hiding this comment

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

that is not how psr-4 works. please check other modules on the proper changes for it.

@@ -16,7 +16,7 @@
"spryker/testify": "*"
},
"autoload": {
"psr-0": {
"psr-4": {
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not how psr-4 works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already known it. I should change my branches quickly and I couldn't find the right way earlier

}

/**
* @param mixed $value
Copy link
Contributor

Choose a reason for hiding this comment

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

If you know you only work with array you can say that here.

* @param int|null $depth
* @param int|null $options
*
* @return mixed|null
Copy link
Contributor

@dereuromark dereuromark Aug 29, 2018

Choose a reason for hiding this comment

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

If you know you only work with array you can say that here maybe. Check if phpstan is ok with this.

interface SevensendersToUtilEncodingServiceInterface
{
/**
* @param array $value
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you already do here :)

* @param int|null $depth
* @param int|null $options
*
* @return mixed|null
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

*/
public function decodeJson($jsonValue, $assoc = true, $depth = null, $options = null)
public function decodeJson($jsonValue, $assoc = true, $depth = null, $options = null): array
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldnt typehint. The doc blocks are still array|null as null can be returned.

@vhrychenko vhrychenko changed the title [WIP] Release 1.0.0 Release 1.0.0 Aug 29, 2018
])->getBody()->getContents());

if (!array_key_exists(static::RESPONSE_KEY_TOKEN, $response)) {
throw new SevensendersApiBadCredentialsException('Bad credentials', 401);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move 401 to constant

@alex-galych alex-galych merged commit 1f8a791 into feature/eco-1924/master-sevensenders Oct 18, 2018
@alex-galych alex-galych deleted the feature/eco-1924/dev-sevensenders branch December 10, 2019 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants