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

Type, Primary key and inconsistency fixes #77

Merged
merged 9 commits into from Oct 24, 2016
Merged

Conversation

kvij
Copy link
Contributor

@kvij kvij commented Oct 17, 2016

The second commit on findId is not that important to me so If you think the regex is overkill leave that out. I do use findId to resolve Division by HID and Exact does not like it when it is forced to be a string.

I am working on lazy loading the deferred but need consistent classnames for that (see lazyload branch). As most inconsistencies where posted recently (in entities that I use even) I wanted to push this out as soon as possible.

Please let me know if there is anything you disagree with.

…bers (like HID) can now be provided but guids are not yet supported.
…d sometimes missing in $fillable.

Added docblock to ReceivableList.
…ass is added as an alias because it is in the release.
Copy link
Member

@stephangroen stephangroen left a comment

Choose a reason for hiding this comment

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

Nice job! Can you check my comments?

@@ -32,6 +32,8 @@ class Division extends Model
{

use Query\Findable;

protected $primaryKey = 'Code';
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix this indentation?

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 just updated Komodo Edit will check its settings.

class ReceivableList extends Model
{

use Query\Findable;

protected $primaryKey = 'HID';
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix this indentation?

class_alias(__NAMESPACE__ . '\Transaction', __NAMESPACE__ . '\Transactions');
Copy link
Member

Choose a reason for hiding this comment

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

You have created an extra class for backward compatibility here but have not done so for the Subscription* classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Subscriptions and SubscriptionTypes and SubscriptionLines are added recently added I dit not think this nessesary. Do you want me to add aliases for these classes as well?

Copy link
Member

Choose a reason for hiding this comment

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

Since they are already released, we should. If you can do that, it would be great :)

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 have just added them. All issues with this pull request should be resolved unless I introduced something in a5da440 or more likely 87a2e70 that you do not approve off.

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 don't see a use case that does not use the Autoloader so I could add a trigger_error statement with a depreciation warning. This way we can in the future clean up the extra files.

…hp and SubscriptionType.php.

Added some properties to SubscriptionLine.
…m the Exact Online reference documentation when visiting the resource details pages.

The generated php code includes the docblock, the primaryKey override (that is often forgotten) and is copy paste ready. The changes in the last commit are created with the script.
Note: Only tested in Firefox with Greasemonkey but should work with Tampermonkey in chrome.
@stephangroen stephangroen merged commit 38d479c into picqer:master Oct 24, 2016
@stephangroen
Copy link
Member

Thanks @kvij :)

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.

None yet

2 participants