Added pelican-gfm plugin #1224

dfoulks1 · 2019-10-11T15:02:14Z

There were a few flake8 exceptions in pelican-gfm:

init.py imports gfm.* but does not call it.
a line in gfm.py is too long (left unshortened)

The python unittest ran without error

Created a Readme.md file for this plugin. 10k foot view

Updated the Readme.rst in the pelican-plugins root.

This plugin references getpelican/pelican#2387

There were a few flake8 exceptions in pelican-gfm: - __init__.py imports gfm.* but does not call it. - a line in gfm.py is too long (left unshortened) The python unittest ran without error Created a Readme.md file for this plugin. 10k foot view Updated the Readme.rst in the pelican-plugins root.

Lucas-C

Thanks for submitting this plugin !
I made a quick review and asked a few questions.
Also, could you provide some tests please ?

Lucas-C · 2019-10-18T11:56:14Z

pelican-gfm/gfmSetup.py

+LIBCMARKLOCATION = "/usr/lib/x86_64-linux-gnu"
+
+ARCHIVES = "https://github.com/github/cmark-gfm/archive"
+VERSION = "0.28.3.gfm.12"


Could you make this a configurable setting maybe ?

I'd much prefer this path is configurable because I might not have those packages at /usr/lib/x86_64-linux-gnu

I've put these values in the Settings.py file so that we can update them without having to update any actual code.

Lucas-C · 2019-10-18T11:58:46Z

pelican-gfm/gfm.py

+        # Optionally we can have the gfmSetup script run here
+        # as root and configure the system
+        # Probably just easier to run it independently tho.
+        sys.exit(1)


Raising an exception would be best, and allow to specify an explicit error message

Lucas-C · 2019-10-18T12:01:15Z

pelican-gfm/gfmSetup.py

+
+def apt_install(package):
+    # I need to be able to do this a better, in a less sudo + apt-y way
+    subprocess.call(["apt-get", "install", package, "-y"])


I don't think those kind of installation steps should be automatically done by plugins.
They'd better be detailed in the plugin README.
What if the those steps require sudo rights on the user machine ? What if he uses apt or yum as a package manager and not apt-get ??

I agree, this is a remnant of when this script was a part of something else. I had mistakenly left it in. It's since been removed.

Lucas-C · 2019-10-18T12:01:29Z

pelican-gfm/gfmSetup.py

+ARCHIVES = "https://github.com/github/cmark-gfm/archive"
+VERSION = "0.28.3.gfm.12"
+LOCAL = "cmark-gfm.$VERSION.orig.tar.gz"
+WORKSPACE = '/tmp/build-cmark'


Better use tempfile.TemporaryDirectiry : https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory

Used tempfile TemporaryDirectory

Lucas-C · 2019-10-18T12:04:37Z

pelican-gfm/Readme.md

+Requirements
+============
+
+pelican-gfm has no requirements that are outside of the python standard library aside from pelican itself.


Out of curiosity, have you tried https://pypi.org/project/gfm/ or https://pypi.org/project/py-gfm/ ?
Do they provide what you need maybe, already bundled as a Python package ?

I believe that there were pieces of the github markup that those packages did not readily provide. I'm not 100% sure as I Inherited the base of this.

Lucas-C · 2019-10-18T12:05:33Z

pelican-gfm/Readme.md

@@ -0,0 +1,22 @@
+pelican-gfm


From my understanding, this plugin will only work on Debian systems.
Better mention this OS limitation in the README

Lucas-C · 2019-10-18T12:06:49Z

pelican-gfm/gfm.py

+OPTS = 0
+
+# The GFM extensions that we want to use
+EXTENSIONS = (


Could this be made configurable as a setting ?

this is also now a setting

trying to make it a little more terse and get rid of some of the junk that was in there. Also putting better documentation in the functions.

did PEP8 verification on the scripts via pycodestyle. they all pass. I also added a vars file so that we can set where the plugin will work.

made sure that the plugin still runs. looks great and serves up just fine. I've pulled most of the configurables out of the script and put it into a vars file. hopefully this will suffice for the pelican folks.

got tempfile.TemporaryDirectory to work out just fine. also checks the config like a champ.

shuttle1987 · 2019-12-17T04:29:54Z

pelican-gfm/gfmSetup.py

+
+            # Move the libcmark.so artifacts in place
+            print("Moving files")
+            subprocess.call([


Is there some reason you can't use shutil.move to move the files here?

good catch! I ended up going with shutil for these moves.

shuttle1987 · 2019-12-17T04:30:14Z

pelican-gfm/__init__.py

@@ -1,3 +1,3 @@
-#!/usr/bin/environment python -B
+#!/usr/bin/environment python333 -B


Is the python333 a typo?

Yes that was a typo.

also removed an erroneous interpreter typo

Will write some tests tomorrow

tried to get this to work when invoked with python or python3. currently works best with python3. something about the tempfile.TemporaryDirectory() call not working. I can handle that another day.

shuttle1987

I feel as though we could use CFFI to handle the interaction with the C library and allow this plugin to be installable in a more cross platform manner. I might give this a go in a future PR, in the interim I've made a few suggestions about creating paths that could help this code be more cross platform.

shuttle1987 · 2019-12-18T07:40:07Z

pelican-gfm/gfmSetup.py

+                     ARCHIVES + "/" + VERSION + ".tar.gz", WORKSPACE,
+                     "-P",
+                     WORKSPACE])
+    subprocess.call(['tar',


Could we replace this subprocess.call to tar -xzf with using the standard library tarfile?

shuttle1987 · 2019-12-18T07:40:46Z

pelican-gfm/gfmSetup.py

+        os.mkdir(WORKSPACE)
+    subprocess.call(["wget",
+                     "--quiet",
+                     ARCHIVES + "/" + VERSION + ".tar.gz", WORKSPACE,


I'd prefer if these paths were created with os.path.join such that we can be more easily able to make this cross platform in the future.

the paths are now made with os.path.join

shuttle1987 · 2019-12-18T07:41:22Z

pelican-gfm/gfmSetup.py

+                     WORKSPACE + "/" + VERSION + ".tar.gz",
+                     "-C",
+                     WORKSPACE])
+    BUILDSPACE = WORKSPACE + "/" + "cmark-gfm-" + VERSION + "/build"


Likewise os.path.join would be good for creating the path here

and os.path.join is used here also

thanks for making these changes

Updated the readme

unconfigured system will no instruct you as to how to configure the system

shuttle1987

If you are looking to make the behavior of Python 2 and 3 consistent with regards to import semantics have a look at PEP 328 which introduced from __future__ import absolute_import. I think this is a better approach than the except ImportError way.

shuttle1987 · 2019-12-19T03:07:15Z

pelican-gfm/gfm.py

-from . import gfmSetup
-from . import Settings
+
+try:


What is the purpose of this block with the relative import? If you are trying to maintain some compatibility with ancient Python versions can you solve this problem with the PEP 328 from __future__ import absolute_import approach?

I got the absolute imports working but now the plugin doesn't conform to your documentation.

shuttle1987 · 2019-12-19T03:08:05Z

pelican-gfm/gfm.py

+
+try:
+    from . import Settings
+except:


If you want to use this approach for imports I think this should only catch an ImportError.
See my above comment about other potential approaches.

I ended up removing this entirely.

shuttle1987 · 2019-12-19T03:08:58Z

pelican-gfm/test.py

+
+import unittest
+
+# python2 and python3 differ on how to do this it seems


See my other comment about from __future__ import absolute_import

to avoid the try / except for importing

Couldn't get thee imports to work consistently.

the format doesn't seem to gel with your contributing a plugin documentation. https://github.com/getpelican/pelican-plugins/blob/master/Contributing.rst

avaris · 2019-12-20T21:14:05Z

pelican-gfm/gfm.py

-from . import gfmSetup
-from . import Settings
+import gfmSetup
+import Settings


Can you show what's not working with:

from . import gfmSetup

That should work.

I'm sorry, I was making notes for myself there. I couldn't get the unittest to work and still have test content be generated. that seems to be corrected now.

avaris · 2019-12-20T21:28:04Z

pelican-gfm/gfmSetup.py

+    t2 = subprocess.Popen([
+                            "grep",
+                            "-q",
+                            package
+                          ], stdout=subprocess.PIPE, stdin=t1.stdout)


So... you're running grep to check for a string in a bunch of lines from python? :)

yeah, that does seem silly. thanks for pointing that out.

Lucas-C requested changes Oct 18, 2019

View reviewed changes

dfoulks1 added 6 commits December 16, 2019 12:18

Made some changes to the pelican gfm setup script

140ab9f

trying to make it a little more terse and get rid of some of the junk that was in there. Also putting better documentation in the functions.

PEP8 verified

9b31101

did PEP8 verification on the scripts via pycodestyle. they all pass. I also added a vars file so that we can set where the plugin will work.

finished up pep8 compliance

44d5b95

made sure that the plugin still runs. looks great and serves up just fine. I've pulled most of the configurables out of the script and put it into a vars file. hopefully this will suffice for the pelican folks.

Updated Readme.md

886009c

Python3-ized everything

284b90e

got tempfile.TemporaryDirectory to work out just fine. also checks the config like a champ.

Moved Extensions definition to gfmVars

03dfb9d

shuttle1987 reviewed Dec 17, 2019

View reviewed changes

dfoulks1 added 9 commits December 17, 2019 08:52

Clarified some of the names for the files.

963e121

updated name

469c99a

pycodestyled the scripts after changes

922db0c

fixed python3 import

962682a

changed some relative pathing import issues

7bb9115

also removed an erroneous interpreter typo

Ensured that the whole thing works

6df3efd

Added OPTS to settings

f9518da

pycodestyled it and ensured it works again.

b897337

Will write some tests tomorrow

Updated readme file

788e89b

tried to get this to work when invoked with python or python3. currently works best with python3. something about the tempfile.TemporaryDirectory() call not working. I can handle that another day.

shuttle1987 reviewed Dec 18, 2019

View reviewed changes

dfoulks1 added 2 commits December 18, 2019 12:26

Added a test for the register / add_readers function.

8b6c56e

Updated the readme

Updated error message

d22316b

unconfigured system will no instruct you as to how to configure the system

shuttle1987 reviewed Dec 19, 2019

View reviewed changes

dfoulks1 added 3 commits December 19, 2019 16:59

Used __future__

a056e7e

to avoid the try / except for importing

Removed absolute imports

76574fb

Couldn't get thee imports to work consistently.

got absolute imports working

6439c43

the format doesn't seem to gel with your contributing a plugin documentation. https://github.com/getpelican/pelican-plugins/blob/master/Contributing.rst

avaris reviewed Dec 20, 2019

View reviewed changes

dfoulks1 added 3 commits December 20, 2019 15:50

updated some string checking.

4a932be

Updated Settings and Docs and some trimmed fat

f57cf1d

OF: Fixed a typo

7e8b15c

MinchinWeb mentioned this pull request Nov 14, 2023

Move to CommonMark MinchinWeb/blog.minchin.ca#133

Open

		@@ -1,3 +1,3 @@
		#!/usr/bin/environment python -B
		#!/usr/bin/environment python333 -B


		import unittest

		# python2 and python3 differ on how to do this it seems

Added pelican-gfm plugin #1224

Are you sure you want to change the base?

Added pelican-gfm plugin #1224

Conversation

dfoulks1 commented Oct 11, 2019

Lucas-C left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucas-C Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuttle1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfoulks1 Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuttle1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucas-C Oct 18, 2019 •

edited

Loading

dfoulks1 Dec 18, 2019 •

edited

Loading