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

Removed from models import* #fixed 1057 #1233

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
3 participants
@ankit01ojha
Copy link
Contributor

commented Jan 21, 2017

Replaced bodhi.server.models package to bodhi.server.models module.

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2017

@bowlofeggs can you please review this :)

@trishnaguha

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

@ankit01ojha Looks like you already opened several PRs for this issue before. When modifying commit it is recommended to do force push instead of closing the PR and opening a new one.

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2017

@trishnaguha I am really sorry for this, I will not do it again( I was new to github, now I am quite used to it).

@bowlofeggs
Copy link
Member

left a comment

We don't want to make changes that are unrelated to renaming the module. Also, the tests do not pass.

@@ -34,7 +34,7 @@


from sqlalchemy import Unicode, UnicodeText, Integer, Boolean
from sqlalchemy import DateTime, engine_from_config
from sqlalchemy import DateTime

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Jan 23, 2017

Member

We shouldn't be making very many modifications to this file, other than merging the Enum items in. This import is needed, and I see a lot of other unrelated changes elsewhere in the file.

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch 3 times, most recently from 4b85803 to af7b9fc Jan 24, 2017

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

@bowlofeggs Modified!

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch from af7b9fc to f79de53 Jan 27, 2017

@@ -158,12 +266,10 @@ def grid_columns(cls):

def update_relationship(self, name, model, data, db):
"""Add items to or remove items from a many-to-many relationship

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 2, 2017

Member

Dropping all of these spaces are not related to the change - can you make sure that only the git mv and merging enum in are happening in this pull request?

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch 2 times, most recently from b1ded3d to e5bd943 Feb 3, 2017

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2017

@bowlofeggs Is this fine or it still needs corrections?

@trishnaguha

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

@ankit01ojha Can you run the testsuite with these changes and update the ticket with the result of your test (Result as in Success or Failing). That's the best way which lets you know if you are doing it right when dealing with large codebase.
To run the testsuite follow the Doc: https://github.com/fedora-infra/bodhi/blob/develop/docs/developer_docs.rst#quick-tips-about-the-bodhi-vagrant-environment

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017

@trishnaguha I tried to do this https://github.com/fedora-infra/bodhi/blob/develop/docs/developer_docs.rst#quick-tips-about-the-bodhi-vagrant-environment as you said but I can't find set of commands in my .bashrc so I ran this $ nosetests -v and here's the output.

screenshot from 2017-02-05 11-42-52

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2017

I am confused does it need changes, as error shown are the files that I have removed.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

I am guessing that the old .pyc file __init__.pyc may still be present on your system.

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch 2 times, most recently from e7f9d08 to 3a5048f Feb 7, 2017

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

@bowlofeggs this time it passed 79% other errors are coming due to the import of bodhi.server.models and its files, so, do I have to make changes in all other file in this commit or I make other commit for them?
screenshot from 2017-02-07 13-53-39

@ankit01ojha ankit01ojha closed this Feb 7, 2017

@ankit01ojha ankit01ojha reopened this Feb 7, 2017

@trishnaguha

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

@ankit01ojha Can you please paste the failed tests in fpaste?

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

@ankit01ojha The issue is that there are many places in the code that import the file that has been deleted, so you will need to adjust the imports so they import the new location. I recommend going through a Python tutorial or two to learn more about modules and import statements. The official Python 2 tutorial is decent, and it has a page about modules:

https://docs.python.org/2/tutorial/index.html
https://docs.python.org/2/tutorial/modules.html

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2017

@bowlofeggs I almost fixed all the import errors, now its 81% passed which is the minimum required.
The rest errors are https://da.gd/CeOkT. Tried to fix them but was not successful.
screenshot from 2017-02-12 00-07-04

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch 2 times, most recently from 547e3e8 to 5fa16d9 Feb 11, 2017

@trishnaguha

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2017

@ankit01ojha You need to follow the documentation that bowlofeggs suggested and fix import path. If you can look in to the failed testcases in the fpaste, it gives you hints exactly where your code is failing.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

@ankit01ojha It looks like there are mock statements that are altering the module you've removed. You should be able to look in the cited test files and find those paths. They're often going to be in statements that look like @patch('bodhi.server.models.models.') over the tests, but they can also appear inside of tests or start with @mock.patch```.

You can read more about mock here:

http://www.voidspace.org.uk/python/mock/

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch from 5fa16d9 to de10649 Feb 15, 2017

@ankit01ojha ankit01ojha force-pushed the ankit01ojha:1057 branch from de10649 to 5886908 Feb 15, 2017

@ankit01ojha

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

Thanks @bowlofeggs and @trishnaguha , now it passed all the test .
screenshot from 2017-02-15 17-07-12

@bowlofeggs
Copy link
Member

left a comment

Nice, this seems to work!

@bowlofeggs bowlofeggs merged commit f870da5 into fedora-infra:develop Feb 17, 2017

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

There were three comments left that referenced the old location, so opted to just fix those in a separate PR: #1293

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.