bitranox commented 4 years ago

Dear Igor, in order to implement PythonClassParser I started to refractor the monolythic init.py, to make things a bit more easy to handle.

dropped Python 2.7 - Python 3.5 Support (if someone needs it, they can use the current release)
cleaned out Python2.7 - 3.5 related code
started to implement strict type hinting
changed Imports to accomplish "Doctest" and Pytest under Pycharm
ovoid cluttering of the namespace on imports
shellscript to run pytest and mypy in a loop () locally without installing
pycodestyle (old: PEP8) testing and cleaning
started to put abc classes for the base classes
relocated /tests directory outside the sources
updated performance test shellscripts
code coverage reports - see : https://codecov.io/gh/bitranox/Arpeggio
adopted pytest-codestyle rules, not to quarrel about the densed function notation You like

Check out the latest commit, I guess from here we can start to work further.

All tests are working, I also found the performance tests - I will check it out later. Your comments are welcome, please let me know !

Robert

igordejanovic commented 4 years ago

Hi Robert. Thanks for you efforts. This PR is really huge and has a lot of unrelated changes. If you plan this to be accepted in the main repo here are some guidelines (this is generally accepted by many OS projects):

Try to be focused with you PRs. E.g. this PR can be split in many smaller PRs each dealing with one particular issue. It is much easier to review and accept/reject piecewise.
Don't reformat code if it is not a clear PEP-8 omission. For example, grammars are written in condensed mode to be more compact and readable.
Don't do code changes to support some particular feature of some particular IDE (e.g. PyCharm).
Try not to break code into multiple modules if not discussed previously.
Each commit should have a clear and consistent log message (this is extremely important). Look here for very good guideline.
Any dev. helper script should be outside of arpeggio package.
Yes, performance are very important. There are some performance tests I use to check if there has been performance regressions with introduction of new features.

You can rework/clean history of this branch using interactive git rebase.

I'm trying to keep Arpeggio as stable as possible as a lot of projects depend on it, that's why I prefer a smaller focused changes and a bit more conservative approach.

I'm willing to drop support for 2.7 in the near future as the Python is reaching end of life. Type hinting is definitely one of the features I'm looking forward as soon as 2.7 support is dropped.

bitranox commented 4 years ago

Igor, no problem - love it or lump it ! You might merge what You like (or discard it), and make some changes Yourself - however, just check it out - there should be nothing broken. First attemts for strict typing are done. The few sprincles of pycharm related strings can be deleted easily. I guess it is a start in the right direction, just take a look at the latest commit - it was heavvily restructured, for instance using abstract classing to make things better to understand and debug. If You dont want it in the main stream, it is also no problem - then I just will use my fork ... just want to give something back. I did not change any method, function or classname - , so for the user everything should be the same like before. All Your points are reasonable, but restructuring and braking it apart, I just dont know how to do it in small steps - if the vast restructuring is done ( I finsihed with that now), then smaller commits of course make sense. My opinion is (well - its not only mine) that a monolythic module with well over 1000 lines is not the ideal structure which can be handled nicely. If You consider, I can just implement the small changes You suggested, no problem - I am willing to follow Your rules, but just from the current stage it was too much to do (for my point of view) yours sincerely Robert

igordejanovic commented 4 years ago

Hi Robert. Will look at it more closely as soon as I get some time. No problem if you don't have time at the moment to do according to the guidelines. I understand it is a lot of work. When I get to it I'll see if I can extract pieces that I find relevant. Thanks for sharing.

bitranox commented 4 years ago

Dear Igor, no problem, I will refractor it to meet Your guidelines. Which line-length is acceptable for You, the original 80 Chars are a bit annoying ... let me know, then I will refractor again. I then let You know when finished, then You might test with textX to find any problems if any - as said before, the user API is still the same. yours sincerely Robert

igordejanovic commented 4 years ago

Thanks. Yup, we are using standard flake8 settings for line length (79) but it is ok to leak here and there (for example with grammars) if it is better visually. I know that it can be sometimes annoying but really helps when working on code side-by-side, or reviewing diffs on laptop monitor. We are enforcing flake8 rules by CI in textX but not in Arpeggio. It would definitely be good to introduce flake8 checks here as well when the time permits.

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b854174). Click here to learn what that means. The diff coverage is 83.06%.

@@            Coverage Diff            @@
##             master      #70   +/-   ##
=========================================
  Coverage          ?   83.23%           
=========================================
  Files             ?       15           
  Lines             ?     1324           
  Branches          ?      238           
=========================================
  Hits              ?     1102           
  Misses            ?      171           
  Partials          ?       51

Impacted Files	Coverage Δ
arpeggio/parser_python_class.py	`10.86% <10.86%> (ø)`
arpeggio/peg_lexical.py	`100% <100%> (ø)`
arpeggio/peg_utils.py	`100% <100%> (ø)`
arpeggio/export.py	`86.45% <100%> (ø)`
arpeggio/parser_peg_clean.py	`100% <100%> (ø)`
arpeggio/arpeggio_settings.py	`100% <100%> (ø)`
arpeggio/peg_semantic_actions.py	`60.71% <60.71%> (ø)`
arpeggio/visitor_base.py	`78.78% <78.78%> (ø)`
arpeggio/error_classes.py	`85.71% <85.71%> (ø)`
arpeggio/parser_base.py	`86.75% <86.75%> (ø)`
... and 5 more

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 b854174...67be9a7. Read the comment docs.

bitranox commented 4 years ago

Ok Igor, please checkout the latest commit - refracturing is finished now, so please give it a look :

shellscript to run pytest and mypy in a loop () locally without installing (under /tests)
pycodestyle (old: PEP8) testing and cleaning
started to put abc classes for the base classes
relocated /tests directory outside the sources
updated performance test shellscripts to be callable from anywere, not only from the current directory, added also timestamp to the report files in order to compare ...
code coverage reports - see : https://codecov.io/gh/bitranox/Arpeggio
adopted pytest-codestyle rules, not to quarrel about the densed function notation You like
dev-requirements.txt to note down requirements needed for development
travis.yml adopted for codecoverage (mypy test not now .... )

Check out the latest commit, I guess from here we can start to work further, with the usual "small step" commits - please let me know.

yours sincerely

Robert

igordejanovic commented 4 years ago

@bitranox Thanks for the contribution! Looks good on a first look. It will take some time for a review though since it is a big change and I would like to make sure that we don't miss anything.

bitranox commented 4 years ago

Dear Igor, added Travis Windows and OsX Testing - check it out. yours sincerely Robert

textX / Arpeggio

Arpeggio Refractor and cleanup #70

Codecov Report