support-offer-files #64

iamanikeev · 2018-03-14T21:50:39Z

No description provided.

codecov · 2018-03-14T21:53:36Z

Codecov Report

Merging #64 into master will decrease coverage by 0.21%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   99.76%   99.54%   -0.22%     
==========================================
  Files          12       13       +1     
  Lines         424      441      +17     
  Branches       49       51       +2     
==========================================
+ Hits          423      439      +16     
- Partials        1        2       +1

Impacted Files	Coverage Δ
pyoffers/api.py	`99.12% <100%> (+0.03%)`	⬆️
pyoffers/models/offer_file.py	`92.3% <92.3%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f3b7e...cb930ce. Read the comment docs.

…by HO

Stranger6667 · 2018-03-14T22:16:59Z

docs/changelog.rst

@@ -22,6 +22,7 @@ Added

 - Introduce Application model. (`iamanikeev`_)
 - Support managing of OfferCategory objects. (`iamanikeev`_)
+- Support managing of OfferFile objects. (`iamanikeev`_)


Please, put it in the Added sub-section under Unreleased

Stranger6667 · 2018-03-14T22:17:24Z

docs/usage.rst

+
+Offer files (Creatives)
+~~~~~~~~


It should be the same length as the previous line

Stranger6667 · 2018-03-14T22:20:55Z

pyoffers/api.py

@@ -99,7 +99,10 @@ def _call(self, target, method, single_result=True, raw=False, **kwargs):
            **kwargs
        )
        self.logger.debug('Request parameters: %s', params)
-        response = self.session.get(self.endpoint, params=params, verify=self.verify)
+        if files:


Lets create kwargs for self.session.request method. Like this:

kwargs = {'url': endpoint, 'params': params, 'verify': self.verify} if files: kwargs.update({'method': 'POST', 'files': files}) else: kwargs['method'] = 'GET' response = self.session.request(**kwargs)

Stranger6667 · 2018-03-14T22:22:13Z

pyoffers/models/offer_file.py

+
+
+class OfferFile(Model):
+


Please, remove extra line

Stranger6667 · 2018-03-14T22:23:38Z

pyoffers/models/offer_file.py

+            'If the file is in the different directory, specify its path via ``local_path``.'
+        file_name = kwargs.get('filename')
+        local_path = kwargs.pop('local_path', file_name)
+        return self._call('create', data=kwargs, files={file_name: open(local_path, 'rb')})


You'd better close the file via context manager for example

Stranger6667 · 2018-03-14T22:24:32Z

pyoffers/models/offer_file.py

+        assert 'filename' in kwargs, \
+            'You must specify ``filename`` argument, which should be a name of a readable file in current directory. ' \
+            'If the file is in the different directory, specify its path via ``local_path``.'
+        file_name = kwargs.get('filename')


Just use a single variable for the file path. It will work in the same way as in any popular library or even in python itself.

Stranger6667 · 2018-03-14T22:28:52Z

tests/conftest.py

+@pytest.fixture(scope='session')
+def offer_file(api):
+    return api.offer_files.create(
+        filename='test-thumbnail.jpg', display='TEST_FILE', type='offer thumbnail', width=200,


What is the purpose of filename and local_path. Is it meaningful to use them together?

Stranger6667 · 2018-03-15T13:59:02Z

docs/changelog.rst

+Added
+~~~~~
+
+- Support managing of OfferFile objects. (`iamanikeev`_)


You shouldn't add new features to already released version. Please, add it under Unreleased section

Stranger6667 · 2018-03-15T14:21:22Z

pyoffers/models/offer_file.py

+    def create(self, **kwargs):
+        assert 'filename' in kwargs, \
+            'You must specify ``filename`` argument, which should be a name of a readable file in current directory.'
+        file_name_w_path = kwargs.get('filename')


There are 3 names for 2 things. filename, file_name_w_path and file_name.
let's do it like this:

def create(self, path, **kwargs): with open(path, 'rb') as fd: if 'filename' not in kwargs: kwargs['filename'] = os.path.basename(path) return self._call('create', data=kwargs, files={kwargs['filename']: fd})

In this case it would be possible for users to specify filename. Note, that setdefault is not used, because if there is a filename then os.path.basename will be called without a reason.

This is similar to what it was before, in this case we have two arguments for the same thing: path and filename. I would rather stick to one argument. Auxiliary variables are just required in order not to call kwargs.get('filename') twice.

Stranger6667 · 2018-03-15T14:28:29Z

pyoffers/utils.py

@@ -53,6 +53,23 @@ def prepend_model(self, value, model):
        return value


+class ValuesDict(dict):


It is overkill, please, remove

Stranger6667 · 2018-03-15T14:30:55Z

tests/models/test_offer_file.py

+    with pytest.raises(FileNotFoundError):
+        api.offer_files.create(
+            display='TEST_FILE', type='offer thumbnail', width=200,
+            height=100, offer_id=438, filename='whatever.txt', local_path='whatever.txt'


local_path is not a relevant kwarg. Please, remove it

Stranger6667 · 2018-03-15T14:32:19Z

pyoffers/models/__init__.py

 )
+
+MANAGER_ALIASES = ValuesDict({


Just put 2 keys here, instead of ValuesDictz

Stranger6667 · 2018-03-15T14:33:31Z

pyoffers/models/__init__.py

@@ -1,4 +1,7 @@
 # coding: utf-8
+from pyoffers.models.offer_file import OfferFileManager  # noqa


from .offer_file import OfferFileManager

* Isort code in tests * Support offer files * Add changelog entry * Isort * Isort * Isort * Don't override update method, since filename updates are not allowed by HO * Inherit update from base ModelManager * Isort!! * Fix documentation, get rid of useless argument * Relative import * Fix changelog entry, remove excess class * Simplify create method * Remove excess test case

Alexander Anikeev added 7 commits February 27, 2018 19:37

Isort code in tests

4bccbc7

Merge branch 'master' of https://github.com/iamanikeev/pyoffers

e1e1533

Merge branch 'master' of https://github.com/Stranger6667/pyoffers

3cdec3b

Merge branch 'master' of https://github.com/Stranger6667/pyoffers

5dd4ad2

Support offer files

3a19d22

Add changelog entry

4328bae

Isort

8dbba6b

Alexander Anikeev added 5 commits March 15, 2018 04:54

Isort

d9d4d9e

Isort

a99a3bc

Don't override update method, since filename updates are not allowed …

da683a5

…by HO

Inherit update from base ModelManager

c927d3f

Isort!!

22320ee

Stranger6667 requested changes Mar 14, 2018

View reviewed changes

Fix documentation, get rid of useless argument

683c262

Stranger6667 requested changes Mar 15, 2018

View reviewed changes

Stranger6667 reviewed Mar 15, 2018

View reviewed changes

Alexander Anikeev added 3 commits March 15, 2018 21:35

Relative import

7b4a2b1

Fix changelog entry, remove excess class

59f3c02

Simplify create method

1ef01f9

Stranger6667 approved these changes Mar 15, 2018

View reviewed changes

Remove excess test case

cb930ce

Stranger6667 merged commit cda27d0 into Stranger6667:master Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support-offer-files #64

support-offer-files #64

iamanikeev commented Mar 14, 2018

codecov bot commented Mar 14, 2018 •

edited

Loading

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 14, 2018

Stranger6667 Mar 15, 2018

Stranger6667 Mar 15, 2018

iamanikeev Mar 15, 2018

Stranger6667 Mar 15, 2018

Stranger6667 Mar 15, 2018

Stranger6667 Mar 15, 2018

Stranger6667 Mar 15, 2018

		@@ -53,6 +53,23 @@ def prepend_model(self, value, model):
		return value


		class ValuesDict(dict):

		@@ -1,4 +1,7 @@
		# coding: utf-8
		from pyoffers.models.offer_file import OfferFileManager # noqa


		Offer files (Creatives)
		~~~~~~~~

		)

		MANAGER_ALIASES = ValuesDict({

support-offer-files #64

support-offer-files #64

Conversation

iamanikeev commented Mar 14, 2018

codecov bot commented Mar 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 14, 2018 •

edited

Loading