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

Deprecate duplicate units class #675

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DannyvdSluijs
Copy link
Contributor

@DannyvdSluijs DannyvdSluijs commented Mar 4, 2025

Background

The \Picqer\Financials\Exact\Units class was added in 2017 in ad75b5c whereas the Unit (singular form) was added in 2019 (59dd598). Following the naming conventions it should be a singular form.

PR Goal

This PR updates the Units class to become an extension of Unit and adds the deprecation trigger and annotation.
Once/If we agree on if this deprecation style is acceptable for this project I can update the other duplicate classes as well. First feedback is much appreciated. Also see below about the other class duplications.

Additional info

There are multiple of these problems which can be discovered by running a bash command to find PHP files containing duplicate @see annotations in the src folder.

find ./src/Picqer/Financials/Exact -type f  -name \*.php | xargs grep '@see https://' -h | sort | uniq -c | grep '^   2'
See all duplicate @see notations

   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=DocumentsDocumentCategories
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=HRMDivisions
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=InventoryItemWarehousePlanningDetails
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=LogisticsUnits
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=ProjectTimeTransactions
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=ReadCRMDocumentsAttachments
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SalesShippingMethods
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SyncDeleted
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SyncInventoryItemWarehouses
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SyncInventoryStockPositions
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SyncInventoryStorageLocationStockPositions
   2  * @see https://start.exactonline.nl/docs/HlpRestAPIResourcesDetails.aspx?name=SyncProjectTimeCostTransactions

reviewing the code base using some exploratory testing, duplication between Units and Unit was discovered and verified to be a duplicate
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