Skip to content
This repository has been archived by the owner on May 1, 2019. It is now read-only.

List of repositories from Github #353

Merged
merged 5 commits into from
Mar 4, 2015
Merged

List of repositories from Github #353

merged 5 commits into from
Mar 4, 2015

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Jan 29, 2015

Hi! This proposal is not a resolution of all problems but in this moment I think that there are only one target..
We make zf.modules usable..
Login, submit repos in the first step.. This proposal tries to "resolve" the rate limit problem.

See

With this PR the list of repos is inconsistente because there are a lot of repos wrong (not zf2 modules).. But works and if you try to submit a wrong repo exists a check for that 馃槃

Now there are already problem if i try to submit a good module I have this expection
Statement could not be executed (23000 - 1048 - Column 'module_id' cannot be null)

But I don't remember the story of the module_id 馃搯

This isn't the final resolution :)

@adamlundrigan
Copy link
Contributor

Are you working on this? I'm about to dig into #352 ([WIP] [RFC] Implement Packagist-style module add process) but I don't want to waste my time if someone else is already doing it

@gianarb
Copy link
Contributor Author

gianarb commented Jan 31, 2015

Hi.. This is only another proposal to fix the problem.. This is very easy.. I don't know.. :) No problem for me I close this PR..

@gianarb gianarb closed this Feb 1, 2015
@gianarb gianarb reopened this Feb 23, 2015
@gianarb
Copy link
Contributor Author

gianarb commented Feb 23, 2015

Hi! I reopen this PR because in my opinion this is a good hotfix to resolve (in a first moment) the Module Load problem.. :)

When the @adamlundrigan feature well be ready we can replace it with new flow! :)

Thanks for your work!

@ins0
Copy link
Contributor

ins0 commented Feb 23, 2015

@gianarb needs a rebase 馃槂

@gianarb
Copy link
Contributor Author

gianarb commented Feb 23, 2015

Ready! :) Thanks @ins0..

Now into "My Modules" step return all my repositories and if you try to load a $repository->isModule() == false return exception..

@gianarb gianarb mentioned this pull request Feb 23, 2015
@gianarb gianarb changed the title [WIP] List of repositories from Github List of repositories from Github Feb 23, 2015
@ins0
Copy link
Contributor

ins0 commented Feb 24, 2015

@gianarb if the check is removed from the list will the repository still checked on the register process?

// edit never mind it does 馃憤

@gianarb
Copy link
Contributor Author

gianarb commented Feb 24, 2015

Yes in this moment this check (isModule) run for each repositories when we call list and after submit the single module..

In this PR I remove the first check.. The list comprises all my repositories (not only module) but works :)

@localheinz
Copy link
Member

@Ocramius

Hang on, I missed something!

@localheinz
Copy link
Member

@gianarb

Looks good to me in order mitigate the API rate limit problem for now, since we still have the check when adding modules, see right here:

@Ocramius

Care to review?

@@ -328,15 +315,8 @@ public function testOrganizationActionRendersValidModulesOnly()
;

$moduleService
Copy link
Member

Choose a reason for hiding this comment

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

Is this mock actually needed?

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius

Only to assert that ZfModule\Service\Module::isModule() is never called!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but that's not critical to this particular test, so it should just be skipped

@localheinz
Copy link
Member

@Ocramius

The only thing we've got at the moment is a not-so-user-friendly handling of when you're trying to submit a non-module: an exception is thrown.

However, @ins0 is working on that at the moment.

@gianarb
Copy link
Contributor Author

gianarb commented Feb 24, 2015

Yes in my opinion this is only a first fix, maybe in the future we can implement #352

@gianarb
Copy link
Contributor Author

gianarb commented Feb 25, 2015

@Ocramius @localheinz have you news about this PR?
Works? It will merge after #421?

@localheinz
Copy link
Member

@Ocramius

I think this can go in - but since I contributed here, I'd prefer you to have a look and merge!

@Ocramius
Copy link
Member

Ocramius commented Mar 4, 2015

It is a go indeed

Ocramius added a commit that referenced this pull request Mar 4, 2015
@Ocramius Ocramius merged commit 3e3c202 into zendframework:master Mar 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants