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

ModTool: Add features and update functionalities #1873

Closed
wants to merge 82 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@swap-nil7
Copy link
Contributor

swap-nil7 commented Jul 5, 2018

This pull request contains the following features:-

  1. A shift of CLI parsing from Argparse to Click. (additional dependency: click)
  2. Plug-in Architecture for ModTool.
  3. Support for external plug-ins. (Additional dependency: click-plugins)
  4. Split of modules into core, cli, templates, tests, and tools.
  5. Python API for ModTool
  6. Tests for ModTool API
  7. Logger for ModTool
  8. ANSI colors to the logger stderr, ModTool Exception for CLI, input statements, ask_yes_or_no, etc.
    (Optional Dependency: colorama)
  9. Replacement of XML Generator by YAML Generator
  10. Update of Templates (mainly addition of yaml default grc template)
  11. Addition of XML to YAML converter as a command line utility as well as an API class.

Other features like SequenceConpleter, replacement of raw open with with open(file) as f: have been also added.

swap-nil7 added some commits May 18, 2018

Plugin Architecture: Multicommand to Group
* click-plugins can be added only to instances of Class click.Group()
* temporarily remove SequenceCompleter from gr_modtool rm
Modtool: Remove unused imports and fix Pylint issues
* Remove the methods from the inherited class CommandCLI that are
  exactly same as the parent class click.Group
* Remove wildcard import from modtool_base
* Fix other Pylint issues with the CLI
* add c++ to the language SequenceCompleter
@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Jul 19, 2018

@marcusmueller: I thought the next was Py3k-only. Shall I import print and unicode_literals from __future__?

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Sep 5, 2018

@swap-nil7 former next and soon-to-be 3.8 release is both Py2 and Py3k. Thus code should be compatible with both.

@primercuervo

This comment has been minimized.

Copy link
Member

primercuervo commented Sep 6, 2018

@marcusmueller @noc0lour Well.. to be fair, that previous statement constitutes some big-deal miscommunication, as at the beginning of GSoC 2018 the work was agreed upon the next branch being Py3k only, and so the work was based on that agreement as well. I ignore how many changes are going to be needed for this PR to be compatible (it might as well just be one line), but I just wanted the record to state that this certainly modifies the (expectingly finished) roadmap for this work.

@swap-nil7 are you up to add this Py2k compatibility?

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Sep 6, 2018

I will try to make it version independent compatible but I don't really see the reason why Py2 compatibility is even needed.

@primercuervo

This comment has been minimized.

Copy link
Member

primercuervo commented Sep 6, 2018

@swap-nil7 thanks for taking care of this. Please do let us know if you need support in any way.

The Py2k compatibility is for GNU Radio in general. If there is a component that requires still Py2k, then we can't just have other components (needless to say one so widely used as gr_modtool) that is Py3k only. We are trying to move all to Py3k, and we will at some point, but as of now it is just a decision to keep compatibility.

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Sep 6, 2018

@swap-nil7 Thanks a lot! If you need any help or get stuck somewhere let us know. @primercuervo I wasn't aware of the original roadmap for gr_modtool.

@primercuervo

This comment has been minimized.

Copy link
Member

primercuervo commented Oct 15, 2018

@swap-nil7 any update on this?

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Oct 15, 2018

@primercuervo: I apologize for the delay but I am not very sure about all the changes that should be incorporated for Py2k compatibility. As of now, I have just imported modules from __future__.

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Oct 16, 2018

@swap-nil7 No problem, I'll take a look at what big chunks are missing 👍

@noc0lour noc0lour self-assigned this Oct 16, 2018

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Oct 16, 2018

I have your latest branch rebased on master and will look for py2k compat

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Oct 16, 2018

Two things we also need (aside from click) to get it running:

  • click-plugins
  • pyyaml with libyaml

The second one is quite hidden in CLoad/CDump calls. Maybe we can make CLoad/CDump optional if libyaml is available, that's how this is handled in GRC as well.

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Oct 16, 2018

This should be all for Py2k compat: Basically only the last commit is needed for the compat. The other commits are fixing other issues on the way (which prohibited me from compiling/running gr_modtool): https://github.com/noc0lour/gnuradio/tree/modtool_rebase the branch is also already rebased on master.

@primercuervo

This comment has been minimized.

Copy link
Member

primercuervo commented Oct 17, 2018

@noc0lour thanks for taking care of this. In GRC what does it fall back to when libyaml isn't available?

Also, in the commit including all the future imports (i.e. a9b88a4 , noc0lour@147f9f9) are they all needed in all those files? I assume they are, but just wanted to ask out of curiosity.

Additionally, PEP-8 isn't all that clear w.r.t multiple imports of the same module, but I believe these all future imports fit in a <80 chars one-liner.

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Oct 18, 2018

@noc0lour: Thanks a lot for your help. Please let me know if I can be of any help. :)

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Oct 18, 2018

Sure thing. I personally really want to have new things and features (and big reworks in the source tree rather sooner than later :) ).
I actually don't know what would be best practice to move forward.
Maybe @marcusmueller could help with his opinion.
In practice you could either take my rebased branch and force-push it onto your branch or I can force-push it onto your branch.
I'd rather not do that (if I accidently delete your history that would be a shame)'

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Nov 2, 2018

@marcusmueller: Why is the PR closed? 😞

@marcusmueller marcusmueller changed the base branch from next to master Nov 2, 2018

@marcusmueller marcusmueller reopened this Nov 2, 2018

@marcusmueller

This comment has been minimized.

Copy link
Member

marcusmueller commented Nov 2, 2018

@swap-nil7 that was completely unintentional. Github just did it because we've removed next, and I didn't re-target this PR to master. Sorry!

@hcab14

This comment has been minimized.

Copy link
Contributor

hcab14 commented Nov 26, 2018

I had a look at https://github.com/noc0lour/gnuradio/tree/modtool_rebase and indeed I was able to create an OOT with YAML definitions. There are two minor issues

  • in CMakeLists.txt the gnuradio version has to be manually changed to 3.8
  • the line category: [tt] should be category: '[tt]', i.e., quotation marks have to be added to the template. Otherwise the category is interpreted as a python list.

Unfortunately I was not able to rebase https://github.com/noc0lour/gnuradio/tree/modtool_rebase to the current master.

@noc0lour

This comment has been minimized.

Copy link
Member

noc0lour commented Jan 3, 2019

Please continue discussion in #2284

@noc0lour noc0lour closed this Jan 3, 2019

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