Skip to content
This repository has been archived by the owner on Jan 20, 2024. It is now read-only.

Support RAML 1.0 resource types declaration #159

Merged
merged 3 commits into from
Oct 1, 2019

Conversation

rgzp
Copy link
Contributor

@rgzp rgzp commented Sep 26, 2019

No description provided.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 460

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.05%) to 80.068%

Files with Coverage Reduction New Missed Lines %
src/NamedParameter.php 1 83.81%
src/Types/TypeValidationError.php 1 90.57%
src/Validator/ContentConverter.php 1 56.25%
src/Utility/TraitParserHelper.php 6 58.82%
Totals Coverage Status
Change from base Build 458: 0.05%
Covered Lines: 3756
Relevant Lines: 4691

💛 - Coveralls

@coveralls
Copy link

coveralls commented Sep 26, 2019

Pull Request Test Coverage Report for Build 466

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 80.142%

Totals Coverage Status
Change from base Build 458: 0.1%
Covered Lines: 12983
Relevant Lines: 16200

💛 - Coveralls

@rgzp rgzp force-pushed the raml10-resource-types branch from c5ed880 to 86364ae Compare September 26, 2019 05:48
@rgzp rgzp closed this Sep 26, 2019
@rgzp
Copy link
Contributor Author

rgzp commented Sep 26, 2019

RAML 1.0 specification declares resource types as array, while 0.8 was array of arrays.

@rgzp rgzp reopened this Sep 26, 2019
src/Parser.php Outdated
$keyedResourceTypes[$i] = $resourceType;
}
}

foreach ($ramlData['resourceTypes'] as $resourceType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks odd that $ramlData['resourceTypes'] is processed twice. This doesn't feel right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, removed unnecessary code.

src/Parser.php Outdated
$keyedResourceTypes[$k] = $t;

foreach ($ramlData['resourceTypes'] as $i => $resourceType) {
if (\is_int($i)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Readability can be improved by extracting the if body in a submethod, eg private function isRaml08(string|int $key): bool. Indentation levels can also be simplified by replacing the else consrtuction with continue as part of the if.

The final code will read something like:

foreach ($ramlData['resourceTypes'] as $key => $resourceType) {
  if ($this->isRaml08($key)) {
    ... // logic to process 0.8 resource types
    continue; 
  }
  ... // logic to process 1.0 resource types
}


...


/**
 * @param string|int $key
 * @return bool
 */
private function isRaml08($key)
{
  return \is_int($key);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. To reduce cognitive complexity we can move out this part of code in "getKeydedResourceTypes" method.
As we need exactly one or two cycles on resourceTypes, there are almost nothing to improve in code itself. Maybe recursive getKeyedResourceTypes call will look better but it leads to unnecessary array_merge for v0.8.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think a recursive approach is the best one here as there is a known and very low number of iterations. A recursive approach can easily clear the way for otherwise broken, over-nested resources.

@rgzp rgzp force-pushed the raml10-resource-types branch from fc87b3d to 2242a0b Compare October 1, 2019 06:12
src/Parser.php Outdated Show resolved Hide resolved
src/Parser.php Outdated Show resolved Hide resolved
@rgzp rgzp force-pushed the raml10-resource-types branch from 2242a0b to 000697e Compare October 1, 2019 10:49
@martin-georgiev
Copy link
Collaborator

@rgzp thank you for this contribution and the speedy changes.

@martin-georgiev martin-georgiev merged commit 3eec4c2 into raml-org:master Oct 1, 2019
@rgzp rgzp deleted the raml10-resource-types branch October 23, 2019 11:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants