Skip to content

Abstract class enforced #521

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 23, 2018
Merged

Abstract class enforced #521

merged 6 commits into from
Apr 23, 2018

Conversation

@DEKHTIARJonathan
Copy link
Member

@DEKHTIARJonathan DEKHTIARJonathan commented Apr 22, 2018

Motivation and Context

With the PR #519 I noticed that the class LayersConfig was used and thought as an abstract class containing various data.

This PR enforce this behavior to ensure that only one class named like this exist in the library. I also have implemented a unittest for it.

@zsdonghao
Copy link
Member

zsdonghao commented Apr 22, 2018

hi, where is the abc from? it is a new dependence? we may need to add it into requirement.txt

@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@DEKHTIARJonathan
Copy link
Member Author

DEKHTIARJonathan commented Apr 22, 2018

Damned this doesn't work with Python 2.7...
I need to fix this ;)

@DEKHTIARJonathan
Copy link
Member Author

DEKHTIARJonathan commented Apr 22, 2018 •
edited
Loading

@zsdonghao abc is one of the core standard module from python 😁
No new dependency is required, its the standard way of doing abstract class and method in python

@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@tensorlayer tensorlayer deleted a comment Apr 22, 2018
@DEKHTIARJonathan
Copy link
Member Author

DEKHTIARJonathan commented Apr 22, 2018

This PR can be merged ;)
@zsdonghao

zsdonghao
zsdonghao approved these changes Apr 23, 2018
View reviewed changes
Copy link
Member

@zsdonghao zsdonghao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

done

@DEKHTIARJonathan
Copy link
Member Author

DEKHTIARJonathan commented Apr 23, 2018

You don't need to install abc @zsdonghao, it's a core module of python

@DEKHTIARJonathan DEKHTIARJonathan merged commit 7d9506d into master Apr 23, 2018
@DEKHTIARJonathan DEKHTIARJonathan deleted the AbstractClass_Enforced branch April 23, 2018 13:57
luomai pushed a commit that referenced this pull request Nov 21, 2018
@DEKHTIARJonathan
Abstract class enforced (#521)
ea2ce16 
* gitignore cleaned and re-organised

* LayerConfig Abstract Class enforced as Abstract

* LayersConfig unittest added

* Py2 and Py3 compatibility issue fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zsdonghao zsdonghao zsdonghao approved these changes

@lgarithm lgarithm Awaiting requested review from lgarithm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DEKHTIARJonathan @zsdonghao