Fix #798: Removed signal related files and depentant methods #825

jayyyin · 2017-10-05T20:21:12Z

Signals removed,
tester changed,
build and decorator files removed
fixes #798

Pomax · 2017-10-05T23:24:50Z

Hi @jayyyin, can you mention which issue this PR addresses, so that we can see which milestone/epics/etc this work relates to?

jayyyin · 2017-10-06T18:30:19Z

@Pomax yep it's this one #798

humphd · 2017-10-12T02:01:03Z

@Pomax, @cadecairos could one of you take a look at this PR? @jayyyin is my student, and this is for the course. It would be great if someone could do a review.

cadecairos · 2017-10-12T17:10:11Z

@jayyyin (edit: sorry for the random tag, jayYin :)) apologies for the silence. We added Snyk to this project to help monitor our dependencies, but due to the nature of the integration, PRs from contributors always fail on Travis and Appveyor.

Since this isn't ideal, or helpful for contributors like yourself, I'm going to remove the automatic running of Snyk from the test scripts, which will unblock you once you rebase.

jayyyin · 2017-10-13T09:45:28Z

@cadecairos ok, so I just need to update my version is that correct?

cadecairos · 2017-10-13T16:41:12Z

@jayyyin yup, now that #852 has landed, a rebase should unblock merging being blocked

cadecairos

nearly there! just a few small changes to make

cadecairos · 2017-10-13T16:49:15Z

network-api/app/networkapi/highlights/apps.py

@@ -6,5 +6,4 @@ class HighlightsConfig(AppConfig):
    verbose_name = 'highlights'


we should be able to remove this file entirely, if we remove the contents (but not the file) of network-api/app/networkapi/highlights/__init__.py

cadecairos · 2017-10-13T16:49:40Z

network-api/app/networkapi/news/apps.py

@@ -6,5 +6,4 @@ class NewsConfig(AppConfig):
    verbose_name = 'news'

    def ready(self):
-        from networkapi.news.signals import setup_signals
-        setup_signals()
+        pass


we should be able to remove this file entirely, if we remove the contents (but not the file) of network-api/app/networkapi/news/init.py

cadecairos · 2017-10-13T16:49:52Z

network-api/app/networkapi/people/apps.py

@@ -6,5 +6,4 @@ class PeopleConfig(AppConfig):
    verbose_name = 'people'

    def ready(self):
-        from networkapi.people.signals import setup_signals
-        setup_signals()
+        pass


we should be able to remove this file entirely, if we remove the contents (but not the file) of network-api/app/networkapi/people/init.py

cadecairos · 2017-10-13T16:50:18Z

network-api/app/networkapi/utility/tests.py

-        sleep(2)
-        self.assertEqual(self.times_called, 1)
+class TestUtilty(TestCase):
+    pass


lets drop this file entirely

humphd · 2017-10-13T16:54:22Z

@jayyyin we haven't covered rebasing yet, so here is what you want to do (NOTE: I assume you have a git remote setup for Mozilla's repo named upstream, so change that if yours is named something else):

git checkout master
git pull upstream master
git checkout RemovingSignals
git rebase master

If you get conflicts at this stage, you can do git rebase --abort and it will roll things back, until I can talk to you and show you how to proceed.

If it works, you can push your code here (note the -f to force it to overwrite the history of this branch):

git push origin RemovingSignals -f

Let me know if you have questions.

jayyyin · 2017-10-13T17:18:51Z

@cadecairos changes have been done and committed, conflicts have been fixed via rebasing and everything looks good to go =D

alanmoo · 2017-10-25T14:09:25Z

@cadecairos is this good to go?

cadecairos

Let's clean up some of the leftover files that we don't need.

jayyyin · 2017-10-29T00:19:40Z

@cadecairos did I miss anything?

jayyyin · 2017-10-29T00:53:48Z

@cadecairos just realized I forgot to add before commiting, last time I committed, silly me..., everything should be good now.

alanmoo · 2017-10-30T14:05:32Z

Looks like this is still failing. Is this based from the latest build of master?

jayyyin · 2017-10-30T20:57:36Z

@alanmoo that's probably the issue, let me fix that

Signals removed, tester changed, build and decorator files removed

jayyyin · 2017-10-30T21:08:39Z

@alanmoo pretty sure I rebased the master correctly and it appears at least one of the tests still fails, not sure what could be wrong now...

alanmoo · 2017-10-30T21:13:10Z

It’s a problem on master. This should be ready for a proper review when another PR lands and we rebase.

jayyyin · 2017-10-30T21:37:24Z

@alanmoo ah I see, so I just need to wait now?

alanmoo · 2017-10-31T14:51:02Z

Checks have passed. @cadecairos is this good now?

jayyyin · 2017-11-21T15:33:32Z

so I've been chatting with @humphd, and have heard that a lot of people have been away, I'd like to follow up with this.

alanmoo · 2017-11-21T15:59:41Z

I believe we've changed the folder structure a bit since this PR was opened, so it's going to take a bit of git manipulation to get this in merge-able shape again. You may want to open another branch off the current master and manually make the changes again.

jayyyin · 2017-11-21T19:13:47Z

@alanmoo alright I'll take a look at the changes needed once I get home

jayyyin · 2017-11-21T22:14:43Z

@alanmoo how would I make a new branch, rebase the fork I have? or somehow re-fork it?
EDIT:
never mind I just rebased the master branch on my current fork and removed the "app" directory as it seems like that was the change done to the directory

jayyyin · 2017-11-22T12:39:35Z

@alanmoo @cadecairos created new pull request here #920

jayyyin mentioned this pull request Oct 5, 2017

Remove Signals for building using the Heroku Build API #798

Closed

humphd changed the title ~~Removed signal related files and depentant methods~~ Fix #798: Removed signal related files and depentant methods Oct 12, 2017

cadecairos suggested changes Oct 13, 2017

View reviewed changes

jayyyin force-pushed the RemovingSignals branch from 901b053 to 817707a Compare October 13, 2017 17:17

cadecairos suggested changes Oct 25, 2017

View reviewed changes

jayyyin added 2 commits October 30, 2017 16:59

Removed signal related files and depentant methods

98dea4f

Signals removed, tester changed, build and decorator files removed

modifications

76879fd

jayyyin force-pushed the RemovingSignals branch from 392612c to 76879fd Compare October 30, 2017 21:00

Merge branch 'master' into RemovingSignals

1b22561

jayyyin mentioned this pull request Nov 22, 2017

Fix #798: Removed signal related files and depentant methods + rebased master, rebranched and redone changes. #920

Merged

jayyyin closed this Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #798: Removed signal related files and depentant methods #825

Fix #798: Removed signal related files and depentant methods #825

jayyyin commented Oct 5, 2017 •

edited

Loading

Pomax commented Oct 5, 2017 •

edited

Loading

jayyyin commented Oct 6, 2017

humphd commented Oct 12, 2017

cadecairos commented Oct 12, 2017 •

edited

Loading

jayyyin commented Oct 13, 2017

cadecairos commented Oct 13, 2017

cadecairos left a comment

cadecairos Oct 13, 2017

cadecairos Oct 13, 2017

cadecairos Oct 13, 2017

cadecairos Oct 13, 2017

humphd commented Oct 13, 2017

jayyyin commented Oct 13, 2017

alanmoo commented Oct 25, 2017

cadecairos left a comment

jayyyin commented Oct 29, 2017

jayyyin commented Oct 29, 2017 •

edited

Loading

alanmoo commented Oct 30, 2017

jayyyin commented Oct 30, 2017

jayyyin commented Oct 30, 2017

alanmoo commented Oct 30, 2017

jayyyin commented Oct 30, 2017

alanmoo commented Oct 31, 2017

jayyyin commented Nov 21, 2017

alanmoo commented Nov 21, 2017

jayyyin commented Nov 21, 2017

jayyyin commented Nov 21, 2017 •

edited

Loading

jayyyin commented Nov 22, 2017

		@@ -6,5 +6,4 @@ class HighlightsConfig(AppConfig):
		verbose_name = 'highlights'

Fix #798: Removed signal related files and depentant methods #825

Fix #798: Removed signal related files and depentant methods #825

Conversation

jayyyin commented Oct 5, 2017 • edited Loading

Pomax commented Oct 5, 2017 • edited Loading

jayyyin commented Oct 6, 2017

humphd commented Oct 12, 2017

cadecairos commented Oct 12, 2017 • edited Loading

jayyyin commented Oct 13, 2017

cadecairos commented Oct 13, 2017

cadecairos left a comment

Choose a reason for hiding this comment

cadecairos Oct 13, 2017

Choose a reason for hiding this comment

cadecairos Oct 13, 2017

Choose a reason for hiding this comment

cadecairos Oct 13, 2017

Choose a reason for hiding this comment

cadecairos Oct 13, 2017

Choose a reason for hiding this comment

humphd commented Oct 13, 2017

jayyyin commented Oct 13, 2017

alanmoo commented Oct 25, 2017

cadecairos left a comment

Choose a reason for hiding this comment

jayyyin commented Oct 29, 2017

jayyyin commented Oct 29, 2017 • edited Loading

alanmoo commented Oct 30, 2017

jayyyin commented Oct 30, 2017

jayyyin commented Oct 30, 2017

alanmoo commented Oct 30, 2017

jayyyin commented Oct 30, 2017

alanmoo commented Oct 31, 2017

jayyyin commented Nov 21, 2017

alanmoo commented Nov 21, 2017

jayyyin commented Nov 21, 2017

jayyyin commented Nov 21, 2017 • edited Loading

jayyyin commented Nov 22, 2017

jayyyin commented Oct 5, 2017 •

edited

Loading

Pomax commented Oct 5, 2017 •

edited

Loading

cadecairos commented Oct 12, 2017 •

edited

Loading

jayyyin commented Oct 29, 2017 •

edited

Loading

jayyyin commented Nov 21, 2017 •

edited

Loading