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

Refactoring #17

Merged
merged 9 commits into from Jul 27, 2017

Conversation

Projects
None yet
2 participants
@hellysmile
Copy link
Owner

hellysmile commented Oct 13, 2015

No description provided.

hellysmile added some commits Oct 13, 2015

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 25, 2017

I think simplifying the CI / support requirements #21 could make the burden on this a bit easier.

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

@tony thanks You so much for get package back to life.
Is there any chance that we can get this merged in-to current master?
This pr saves a lot of readability for switching from a lot of parameters to kwargs (not sure is it good idea or not).

But main thing here - start support jinja templates same way as django after in-depth refactoring of jinja extension

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

Maybe it will just easier to copy-paste this jinja tag in-to master?

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 27, 2017

Do you want to keep your cache refactor, though? That takes up a big portion of the PR.

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

I can not push You to do it, but include version of django-activeurl in-to cache key can be very useful for someone during upgrade, especially when behaviour of packages is changed due awesome ignore_params options...

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 27, 2017

It'd be good to have push access or to be added to the issue so I can fix the commit w/ you

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

Invitation is send, all write access will be granted, except master branch which is becomes protected right now.

@tony tony force-pushed the refactoring branch from cc5a898 to d682243 Jul 27, 2017

@tony tony referenced this pull request Jul 27, 2017

Merged

Refactor tests #29

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

Some missing things

We need setup.py classifier regarding Python3.6

'open' is setup.py must be 'io.open' ,

All if statements must use new bool helper

@tony tony force-pushed the refactoring branch from d682243 to a67d6bd Jul 27, 2017

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 27, 2017

Updated

All if statements must use new bool helper

Can you give me some examples of this by commenting the code?

Repository owner deleted a comment from coveralls Jul 27, 2017

Repository owner deleted a comment from coveralls Jul 27, 2017

Repository owner deleted a comment from coveralls Jul 27, 2017

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

This landscape and covverals goes crazy, I think we need to remove them and setup codecov service

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 27, 2017

I prefer codecov too. Let's add it to refactor-tests #29

@hellysmile

This comment has been minimized.

Copy link
Owner

hellysmile commented Jul 27, 2017

If statment comment is merge diff missunderstood

Can You merge refactring tests to this branch and lets see how it goes green together

@tony

This comment has been minimized.

Copy link
Collaborator

tony commented Jul 27, 2017

I would prefer merging #29 into master first (it looks neater since I like to cite it at https://devel.tech/site/open-source), I'm almost finished with it.

Repository owner deleted a comment from coveralls Jul 27, 2017

Repository owner deleted a comment from coveralls Jul 27, 2017

Repository owner deleted a comment from coveralls Jul 27, 2017

Repository owner deleted a comment from coveralls Jul 27, 2017

tony added some commits Jul 27, 2017

@tony tony merged commit 1f9460e into master Jul 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tony tony deleted the refactoring branch Jul 27, 2017

Repository owner deleted a comment from landscape-bot Jul 28, 2017

Repository owner deleted a comment from landscape-bot Jul 28, 2017

Repository owner deleted a comment from coveralls Jul 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment