Skip to content

Conversation

@aik099
Copy link
Member

@aik099 aik099 commented Jul 15, 2013

This would be initial version of #21 issue.

Later we can replace type-parsing related code with Typical seamlessly.

@mindplay-dk
Copy link
Member

I already put a lot of work into the reduction-branch of Typical, so I don't want to merge this - not because I expect there's anything wrong with it, but because I don't want to release with a temporary feature, already knowing that backwards compatibility will break with the final release.

If this works for you, I suggest you run with it - deploy your own fork and use it, see how it works out in practice.

Sorry, but I just don't have time to even review this in detail right now.

I did glance over it, and the only thing I would point out is that isScalar() may be something of a misnomer - scalar types are strings, integers, floats, booleans, while types like mixed, array, resource etc. are simple (pseudo) types but not technically scalar types.

@aik099
Copy link
Member Author

aik099 commented Jul 17, 2013

I already put a lot of work into the reduction-branch of Typical, so I don't want to merge this - not because I expect there's anything wrong with it, but because I don't want to release with a temporary feature, already knowing that backwards compatibility will break with the final release.

As I said it's not a Typical replacement, but something that can be used in meanwhile, while Typical reduction branch isn't merged into Annotation library. Also AnnotationContext itself is something that was a part of our plan and Typical should be connected through it. Of course I don't suggest releasing before Typical reduction branch merge.

Any estimates on when Typical reduction branch can be joined with Annotation library?

I did glance over it, and the only thing I would point out is that isScalar() may be something of a misnomer - scalar types are strings, integers, floats, booleans, while types like mixed, array, resource etc. are simple (pseudo) types but not technically scalar types.

Here isScalar method was only to detect if it's a class/interface or something else. Maybe notClassOrInterface would be a better name here.

Off topic:
Do you plan to automate unit tests at some point to ensure that changing code in future won't break anything? Right now they aren't using PHPUnit at all and that's why integration with TravisCI/CoverallsIO doesn't seem possible.

@mindplay-dk
Copy link
Member

Any estimates on when Typical reduction branch can be joined with Annotation library?

Not until I fix the type-definition parser, which may need a rewrite - it's a mess.

Maybe notClassOrInterface would be a better name here.

If you want the terminology to be match Typical, actual classes are considered "complex", while all other types are considered "simple" - so isSimpleType() or isComplexType() depending on your preference.

I have never used TravisCI - if it only works with PHPUnit, I probably never will, as I tend to avoid the bloat of PHPUnit if at all possible. Sorry, but I'm not a fan. Remembering to run the unit-tests before I commit is not a big deal for me, and I use the browser-based test-runner repeatedly while developing - if I had to wait 5-10 seconds for every test-run, I would lose my mind.

@aik099
Copy link
Member Author

aik099 commented Jul 17, 2013

Not until I fix the type-definition parser, which may need a rewrite - it's a mess.

Then I'll be waiting for that moment. Hopefully this will happen this year.

I have never used Travis CI - if it only works with PHPUnit, I probably never will, as I tend to avoid the bloat of PHPUnit if at all possible.

Travis CI can work with anything, but you need to figure out how to connect your testing suite manually using their docs.

as I tend to avoid the bloat of PHPUnit if at all possible. Sorry, but I'm not a fan.

Fan or not, but writing you own testing suite and ignoring any testing frameworks on the market is kind of reinventing a wheel and is counter productive in my opinion. Of course no framework (even testing ones) are perfect, but you can use same approach as you did with Composer: find where it falls short and don't use that and only use what you think works right.

Remembering to run the unit-tests before I commit is not a big deal for me.

But I can't guarantee, that anybody, that would do PR to your library work not forget that. Automating all this would allow to save time when reviewing PR and clearly see when tests are failing right on GitHub.

I use the browser-based test-runner repeatedly while developing - if I had to wait 5-10 seconds for every test-run, I would lose my mind.

It seems, that you don't like command line tools much, since both PHPUnit and Travis CI produce command line output that you can use later to see why Travis CI build failed.

@mindplay-dk
Copy link
Member

writing you own testing suite and ignoring any testing frameworks on the market is kind of reinventing a wheel

Yeah, totally - but if all you need (or want) is a wheel, and not an interplanetary star cruiser, sometimes a little bit of reinventing the wheel is necessary.

I have neither the time nor the motivation to wrestle a giant machine with a million moving parts just to do some basic assertions.

But I can't guarantee, that anybody, that would do PR to your library work not forget that

For the time being, no one (except you) has contributed anything, so I'm not really concerned about that.

Contributing to any project should involve running the test-suite, if one is available - I wouldn't accept pull requests from someone who can't be f*cked to even run the tests in the first place.

It seems, that you don't like command line tools much

I use them when I need to - in the case of running unit-tests, I don't need to.

I would like to move the test-suite to funit eventually, which does support command-line output and might integrate better with a CI server.

But again, time and motivation: same reason I don't move to PHPUnit - and that would require 10 times the time and motivation...

I'm not going to invest my time rewriting the entire test-suite (which already works fine) for some dreamed-up scenario, such as a whole bunch of people suddenly submitting pull-requests. I don't really even have time to maintain this package at all, and if or when I do have some free time on my hands, I'm going to invest it in things that actually matter to the majority of consumers of the package, rather than the minority of (so far only 2) developers.

For that matter, and don't take this the wrong way, but I'd rather not waste my time even debating it...

@aik099
Copy link
Member Author

aik099 commented Jul 17, 2013

:)

@aik099
Copy link
Member Author

aik099 commented Feb 4, 2014

After a bit of talking we've decided that the only context the Annotation can have is a file, so this code (in PR) can be simplified to have IAnnotationFileAware interface with setFile(AnnotationFile $file) method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried to remove whole AnnotationContext file and this method then would end up in AnnotationFile class. Is it ok, @mindplay-dk ? I thought that AnnotationFile is data structure only and shouldn't contain any logic to work on that data.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's not a data structure, then AnnotationFile::resolveType($raw_type) sounds good, because here we're resolving raw data type using that file object (namespace, uses).

@aik099 aik099 mentioned this pull request Feb 5, 2014
@mindplay-dk
Copy link
Member

The only problem with this, is that annotation types will be forced to keep an internal reference to AnnotationFile since they don't have all the information available at once - the file context and the values they need in order to initialize.

What I'm saying is, AnnotationFile is only useful to an annotation type for one purpose: initialization.

So the semantics here are kind of broken. We really ought to refactor, in such a way that all the context information required to initialize, is delivered in one call, so you don't have to store parts of the context in intermediary properties.

That would be a breaking change though, so I understand if you want to leave it as-is for the next release.

On a related note, PropertyAnnotation and VarAnnotation etc. don't use this interface yet. I guess they're still stubs, and all the unimplemented stub annotation-types probably ought to be moved out of master and into a feature-branch? Since technically, this is an incomplete feature.

@aik099
Copy link
Member Author

aik099 commented Feb 5, 2014

If we want to have 1 method for initialization then we run into method conflict where 2 interfaces (IAnnotation and IAnnotationFileAware) will have the init method, but with different declarations. Then the annotation class must implement one or another. Since the VarAnnotation extends the base Annotation class which is already is implementing the IAnnotation interface then we end up in method naming conflict.

@aik099
Copy link
Member Author

aik099 commented Feb 5, 2014

But I understand the idea, that annotation class objects don't need to keep state.

f you wish I can actually connect to you via skype (text messaging). This won't be public as GitHub, but will surely be faster when discussing large things. My skype account is: aik099.

aik099 pushed a commit that referenced this pull request Feb 6, 2014
Initial implementation of AnnotationContext idea. Will rethink the implementation in next feature release alongside with some other architectural changes.
@aik099 aik099 merged commit 6dec9c6 into php-annotations:master Feb 6, 2014
@aik099 aik099 deleted the annotation-context-feat branch February 8, 2014 15:02
@aik099 aik099 added this to the 1.2.0 milestone Aug 17, 2014
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.

2 participants