Skip to content
This repository has been archived by the owner on Feb 23, 2020. It is now read-only.

#744 Use Option for pdf price list #748

Merged
merged 6 commits into from
Jul 12, 2019
Merged

#744 Use Option for pdf price list #748

merged 6 commits into from
Jul 12, 2019

Conversation

ArtemijRodionov
Copy link
Contributor

Closes #744

@ArtemijRodionov ArtemijRodionov self-assigned this Jul 10, 2019
Copy link
Contributor

@duker33 duker33 left a comment

Choose a reason for hiding this comment

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

minor naming and code style

self.assertIsInstance(self.context['products'], models.OptionQuerySet)
self.assertGreater(self.context['products'].count(), 100)

def test_tags_group(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_tags_group(self):
def test_tags_are_grouped(self):

And we can get rid of additional comment

self.assertEqual(group, tag_.group)

# it is products' tags
_, grouped_tags = zip(*self.context['group_tags_pairs'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_, grouped_tags = zip(*self.context['group_tags_pairs'])
grouped_tags = [tags for _, tags in self.context['group_tags_pairs']

Would be more readable in my taste.

or even the whole expression

tags = set([t for tags for _, tags in self.context['group_tags_pairs'] for t in tags])

Up to you

@duker33
Copy link
Contributor

duker33 commented Jul 10, 2019

@artemiy312 , let me comment on our microtasking style. Here you've done contexts refactoring which has nothing common with solving issue with pdf price list.

What seems to me as more clear decision:

  • create task to refactor a piece of code directly with Github
  • you should be rewarded on task creating. So you log astro time as we are having no autorewarding for tasks system
  • block the current task about pdf prices since it'll use the future refactored code
  • resolve the fresh created task

It's doesn't matter in this concrete case, but we should develop solid microtasking style for the future 💪

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix pdf price generation
2 participants