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

Move logger into own class and separate from main script #8

Merged
merged 8 commits into from
Dec 14, 2016

Conversation

pleonex
Copy link
Contributor

@pleonex pleonex commented Nov 15, 2016

Note: This branch contains changes from the formatter branch. Please, don't review this PR until #7 is accepted and merged into master

The main script logic was inside the main log parser module. This Pull Request converts the LogParser logic into a class that can be used later for many other scripts. The current main script will execute the LogParser class as always. This allows to use this application as a library too by importing the logparser package.

@iblancasa
Copy link
Contributor

I will review this after we close the PR #7

Copy link
Contributor

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

Some small changes

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Package LogParser."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Package LogParser or LogParser Package?

@@ -19,7 +19,7 @@
+ get_regex_list: Get the regular expressions and function list.
"""
from __future__ import absolute_import
import debug.debug as debug
import logparser.debug.debug as debug
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do something with this

Copy link
Contributor Author

@pleonex pleonex Dec 13, 2016

Choose a reason for hiding this comment

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

We will discuss the import statements in the log files in #13.

for kind in filter(None, content.get('kind', '').split("|")):
for subkind in KIND_TO_COLOR[kind].split("|"):
color += COLORS[subkind]
if len(color) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not necessary the comparation

if len(color):


def countset_add_element(countset, el):
"""Add an element to the countset."""
if el not in countset:
Copy link
Contributor

Choose a reason for hiding this comment

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

You are doing this because you don't want have "el" repeated in the countset, true? So, we should use a set, not a list:
https://docs.python.org/2/library/stdtypes.html#set

(countset --> count-set --> count set. Oh! Wait!! It is a set!) 💃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CountSet is a class that count the number of times that a given entry is added. Every entry is in the object once and it has an ID and count. We will refactor that code for #12.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect

for typ in stats[addr]:
# If this is a port with dictionary of statistics types
if isinstance(stats[addr][typ], dict):
# Show statistics per port with verbosity >= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this

@pleonex
Copy link
Contributor Author

pleonex commented Dec 13, 2016

Applied code review changes. Thanks for the feedback!


def countset_add_element(countset, el):
"""Add an element to the countset."""
if el not in countset:
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect

@iblancasa iblancasa merged commit 20a595c into master Dec 14, 2016
@pleonex pleonex deleted the classes branch December 14, 2016 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants