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

fix: prefix property starting with number with 'n' #584 #587

Merged
merged 1 commit into from Feb 1, 2022

Conversation

Gounlaf
Copy link
Contributor

@Gounlaf Gounlaf commented Jan 15, 2022

No description provided.

@Gounlaf
Copy link
Contributor Author

Gounlaf commented Jan 28, 2022

@Korbeil could you please explain me how you re-generate all data for tests?
The process is not quite clear for me =/

@Korbeil
Copy link
Member

Korbeil commented Jan 28, 2022

@Korbeil could you please explain me how you re-generate all data for tests? The process is not quite clear for me =/

Hey @Gounlaf, sure !
First thing: run the tests, it will fill all the "generated" folders, once that's done and if you added a new fixture test, you have to run the .replace-all-expected-fixtures.sh script on this repository root to replace all expected folders with their respective generated folder.

@Gounlaf Gounlaf changed the title [WIP] fix: prefix property starting with number with 'n' #584 fix: prefix property starting with number with 'n' #584 Jan 30, 2022
@Gounlaf
Copy link
Contributor Author

Gounlaf commented Jan 30, 2022

@Korbeil it should be OK now

@@ -111,41 +111,41 @@ public function setTotalCount(int $totalCount) : self
*
* @return int
*/
public function get1() : int
public function getN1() : int
Copy link
Member

Choose a reason for hiding this comment

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

Getters & setters should still be get1() and set1()

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 face this problem:

  • getName() can't be used because it the original name of the property; one the side effect is, for property like "+1" and "-1" (in the github dataset), it's cleaned as "1"; it generate two getters/setters with the same name
  • using getPhpName, it contains the prefix.
  • I put the prefix with "n" in the Naming class to avoid do it in many places...

What should I do?

  • doesn't apply the cleaning in getPropertyName? it will not be stored in and returned by getPhpName
  • or maybe add a new property on the Property class?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth having a new property on Property class, one for properties and one for methods :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger 👌

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 had to make Naming::cleaning() public.

src/Component/JsonSchema/Jane.php Outdated Show resolved Hide resolved
src/Component/OpenApiCommon/JaneOpenApi.php Outdated Show resolved Hide resolved
@Korbeil
Copy link
Member

Korbeil commented Feb 1, 2022

Thanks a lot for your contribution, could you add a note about it in the CHANGELOG file pls ?

@Gounlaf
Copy link
Contributor Author

Gounlaf commented Feb 1, 2022

Thanks a lot for your contribution, could you add a note about it in the CHANGELOG file pls ?

Yes.
And I will squash if it's OK for you.

@Gounlaf Gounlaf force-pushed the gh-issue-584 branch 2 times, most recently from 533910b to bb22447 Compare February 1, 2022 16:34
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Florian Levis <levis.florian@gmail.com>
@Korbeil
Copy link
Member

Korbeil commented Feb 1, 2022

Thanks again for this contribution !

@Korbeil Korbeil merged commit cc100dc into janephp:next Feb 1, 2022
@Korbeil Korbeil mentioned this pull request Feb 3, 2022
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.

None yet

2 participants