More config values for Runcstone components #867

vasiljevic · 2019-08-04T12:12:56Z

This is a continuation of the work done in PR #592. Two new config values (configurable in conf.py) are introduced:

mchoice_compare_button_show - if False, hide the 'Compare me' button in mchoice (default True)
fitb_compare_button_show - if False, hide the 'Compare me' button in fitb (default True)

bnmnetp · 2019-08-04T16:52:10Z

Thanks,

These are useful configuration parameters, especially for someone running their own book on their own server. When used in the Runestone.Academy mode, then these don't help, as everyone uses the same book, but with different configuration parameters populated into eBookConfig when the page is served.

We are working hard toward keeping our documentation up to date as well as improving our coverage on the selenium based unit tests for each component.

Could you please update the docstrings for the affected directives, and where possible add a unit test to exercise your new changes.

Thanks.

vasiljevic · 2019-08-04T19:34:34Z

@bnmnetp docstrings updates are in the second commit of the pull request.

I've took a look to existing test cases and I'm not sure how to alter config values (in conf.py) between test cases. Existing test cases should be enough for testing default config values (components behaves like before for default config values), but for more detailed testing we need different builds for different test cases.

bjones1 · 2019-08-07T15:45:43Z

I agree. One way to do this is to add a directory named RunestoneComponents/runestone/fitb/test_1 which is mostly a copy of RunestoneComponents/runestone/fitb/test, but has updated conf.py values and updated test_fitb.py contents to make sure the Compare Me button isn't rendered.

bnmnetp · 2019-08-07T15:49:29Z

@bjones1 -- This seems like it would be another use case for the context manager we did for the settings with @presnick

We would need to generalize it a bit but it seems much better than duplicating everything.

bnmnetp · 2019-08-07T15:55:17Z

Alternatively, I think we can also use the -c option to sphinx-build to specify an alternative config.py file. We would just need some way to tell the tests to rebuild

vasiljevic · 2019-08-08T13:20:20Z

@bjones1 @bnmnetp We may also consider to alter config values in build-time between different fitb directive visitings. There are several ideas how to do that: make special testing-purposed directive to alter app.config/env.config (app.config['fitb_compare_button_show'] = ...) or use a callable value as a config value (def fitb_compare_button_show(c): ... instead of fitb_compare_button_show = ...) or both. I've tool a look to the Sphinx source, both __setitem__ and implicit invoke of a callable value in __getattr__ are impmeneted in the Config class: https://github.com/sphinx-doc/sphinx/blob/44c49f462c91870c0ef5e3e7dba74cba3edb8e12/sphinx/config.py#L246.

We in the Loop Foundation (Fondacija Petlja) are currently focused on new and updated online books for the new school year and will be able to continue our work on compatibility PRs in a month or two (including test cases in this PR). Our plan is to become compatible with the original RunestoneComonents (with our additional extensions) and stop to depend on the forked repo.

BTW, Our extensions are also open sourced and we will be ready to port-back some features to RunestoneComponets. We are going to publish English version of some of our online books and then we will be able to show how those additional features work in practice.

bnmnetp · 2019-08-08T14:15:33Z

@vasiljevic That sounds great. The more we can collaborate the more progress we can all make. I found your site (https://petlja.org) and I hope can adopt some of the nice updates you have made to the user interface.

bjones1 · 2019-08-09T17:20:22Z

Hmmm, I like the callable approach -- it would allow us to change the config value on a per-problem basis. I could image creating a directive called update-config or something, with use like:

.. update-config::
    :config_value_name: updated_value

That should be pretty easy to implement. Let me know if you want help doing that -- I've written several directives. The directive might need to modify a callable, as you mention, since I seem to recall that the app.config values aren't (easily) changable.

bnmnetp · 2019-09-17T21:21:52Z

Hi @vasiljevic I just wanted to come back to this, as my semester has started and I'm getting into a schedule. I would love to collaborate with you on your changes and take advantage of some of the great work you have done on improving the components.

How is your schedule these days?

vasiljevic · 2019-10-14T13:37:39Z

Hello @bnmnetp, sorry for my late response. Our new team member @maric993 is now assigned to this pull request. He currently works on test cases and will continue to work on authoring tools including further contributions to the upstream project.

BTW, here is our interactive book in Englih: https://petlja.github.io/TxtProgInPythonEng/ (repo: https://github.com/Petlja/TxtProgInPythonEng)

Late summer and autumn is our annual peak of activity since we provide learning materials and teacher training for the new school year.

bnmnetp · 2019-10-28T13:30:46Z

this is great, thanks.

@bjones1 can you take a look at this one?

bjones1 · 2019-10-28T14:38:15Z

Will do.

bjones1 · 2019-10-31T14:48:23Z

runestone/__init__.py

@@ -44,6 +44,9 @@ def runestone_extensions():
    modules = [ 'runestone.{}'.format(x) for x in module_paths if os.path.exists('{}/__init__.py'.format(os.path.join(basedir,x)))]
    # Place ``runestone.common`` first, so it can run init code needed by all other modules. This assumes that the first module in the list is run first. An alternative to this to guarantee this ordering is to call ``app.setup_extension('runestone.common')`` in every extension.
    modules.insert(0, modules.pop(modules.index('runestone.common')))
+	# ``runestone.updateConfig`` is reserved for testing and will only be included if you are in a directory with ``test`` in its name


This comment doesn't match the code -- it's only included if test is the second to last element of the path.

Fixed the wording to more accurately match the code

bjones1 · 2019-10-31T14:49:39Z

runestone/__init__.py

@@ -44,6 +44,9 @@ def runestone_extensions():
    modules = [ 'runestone.{}'.format(x) for x in module_paths if os.path.exists('{}/__init__.py'.format(os.path.join(basedir,x)))]
    # Place ``runestone.common`` first, so it can run init code needed by all other modules. This assumes that the first module in the list is run first. An alternative to this to guarantee this ordering is to call ``app.setup_extension('runestone.common')`` in every extension.
    modules.insert(0, modules.pop(modules.index('runestone.common')))
+	# ``runestone.updateConfig`` is reserved for testing and will only be included if you are in a directory with ``test`` in its name
+    if('test' not in (os.path.split(os.getcwd())[-1])):


Awkward. There's an unnecessary set of parens around the if statement. Also, use != since you're comparing to a single string, not all elements of the path.

Cleaned up the code. Is there a solution you would recommend which would be better suited for separating this directive from the rest? Is it necessary to do that. I was going off of

make special testing-purposed directive

brought up in this thread previously

Great! Please push your latest commits so I can see them.

I like your approach -- create a special-purpose testing directive, which is only available in a testing environment. I'm just nit-picking the implementation.

bjones1 · 2019-10-31T14:49:51Z

runestone/__init__.py

@@ -44,6 +44,9 @@ def runestone_extensions():
    modules = [ 'runestone.{}'.format(x) for x in module_paths if os.path.exists('{}/__init__.py'.format(os.path.join(basedir,x)))]
    # Place ``runestone.common`` first, so it can run init code needed by all other modules. This assumes that the first module in the list is run first. An alternative to this to guarantee this ordering is to call ``app.setup_extension('runestone.common')`` in every extension.
    modules.insert(0, modules.pop(modules.index('runestone.common')))
+	# ``runestone.updateConfig`` is reserved for testing and will only be included if you are in a directory with ``test`` in its name
+    if('test' not in (os.path.split(os.getcwd())[-1])):
+        modules.pop(modules.index('runestone.updateConfig'))


Use modules.remove.

bjones1 · 2019-10-31T15:08:23Z

runestone/common/project_template/conf.tmpl

@@ -297,3 +297,5 @@ htmlhelp_basename = 'PythonCoursewareProjectdoc'
 #shortanswer_optional_div_class = 'journal alert alert-success'
 #showeval_div_class = 'runestone explainer alert alert-warning'
 #tabbed_div_class = 'alert alert-warning'
+#mchoice_compare_button_show = True


Add a brief description of what this option controls for both of these.

In fitb.py and multiplechoice.py there are these two comments:

mchoice_compare_button_show - if False, hide the 'Compare me' button (default True)

fitb_compare_button_show - if False, hide the 'Compare me' button (default True)

Should I add these to conf.tmpl as well?

Yes, exactly.

bjones1 · 2019-10-31T15:18:04Z

runestone/updateConfig/test/test_update.py

+
+        self.find_fitb("fill2")
+        button_after_config_update = None
+        try:


I'd suggest writing something like:

def is_element_present_by_id(self, id): try: self.driver.find_element_by_id(id) return True except NoSuchElementException: return False

Then you can write

assert self.is_element_present_by_id("question1_bcomp") assert not self.is_element_present_by_id("question2_bcomp")

bjones1 · 2019-10-31T15:21:59Z

runestone/updateConfig/update.py

+    update config
+    """ 
+    required_arguments = 0
+    optional_arguments = 1


Why does this have an optional argument? What is it?

Previous drafts of the directive had optional arguments. Changed it to 0.

bjones1 · 2019-10-31T15:24:35Z

runestone/updateConfig/update.py

+def setup(app):
+    app.add_directive('update-config', UpdateConfig)
+
+class UpdateConfig(RunestoneDirective):


I wouldn't inherit from a RunestoneDirective -- that's means for directives which create a question. This is something completely different.

I am not sure what class to inherit in UpdateConfig do you have any suggestions?
Is Directive a better solution?

Yes, exactly -- inherit from docutils.parsers.rst.Directive.

bjones1 · 2019-10-31T15:24:48Z

runestone/updateConfig/update.py

+        if 'set_config_option' in self.options:
+            config_opt , value = self.options['set_config_option'].split(" ", 1)
+
+            if(len(value)>1000):


Extra parens. Plus, why this limit?

extra parens removed

from Abstract Syntax Trees doc. :

It is possible to crash the Python interpreter with a sufficiently large/complex string due to stack depth limitations in Python’s AST compiler.

Just wanted a safeguard against such occurrence

That makes sense -- would you add a comment to explain this?

updates to: - testing element presence - comment wording and implementation - general code cleanup

bjones1 · 2019-11-01T15:42:21Z

runestone/__init__.py

@@ -44,6 +44,9 @@ def runestone_extensions():
    modules = [ 'runestone.{}'.format(x) for x in module_paths if os.path.exists('{}/__init__.py'.format(os.path.join(basedir,x)))]
    # Place ``runestone.common`` first, so it can run init code needed by all other modules. This assumes that the first module in the list is run first. An alternative to this to guarantee this ordering is to call ``app.setup_extension('runestone.common')`` in every extension.
    modules.insert(0, modules.pop(modules.index('runestone.common')))
+	# ``runestone.updateConfig`` is reserved for testing and will only be included if you are in a directory with ``test`` in its name
+    if('test' not in (os.path.split(os.getcwd())[-1])):


Great! Please push your latest commits so I can see them.

I like your approach -- create a special-purpose testing directive, which is only available in a testing environment. I'm just nit-picking the implementation.

bjones1 · 2019-11-01T15:42:44Z

runestone/common/project_template/conf.tmpl

@@ -297,3 +297,5 @@ htmlhelp_basename = 'PythonCoursewareProjectdoc'
 #shortanswer_optional_div_class = 'journal alert alert-success'
 #showeval_div_class = 'runestone explainer alert alert-warning'
 #tabbed_div_class = 'alert alert-warning'
+#mchoice_compare_button_show = True


Yes, exactly.

bjones1 · 2019-11-01T15:44:21Z

runestone/updateConfig/update.py

+def setup(app):
+    app.add_directive('update-config', UpdateConfig)
+
+class UpdateConfig(RunestoneDirective):


Yes, exactly -- inherit from docutils.parsers.rst.Directive.

bjones1 · 2019-11-01T15:44:47Z

runestone/updateConfig/update.py

+        if 'set_config_option' in self.options:
+            config_opt , value = self.options['set_config_option'].split(" ", 1)
+
+            if(len(value)>1000):


That makes sense -- would you add a comment to explain this?

maric993 · 2019-11-02T09:07:51Z

Let me know if we can improve anything else.

bjones1 · 2019-11-04T17:57:45Z

runestone/__init__.py

@@ -70,7 +70,13 @@ def runestone_extensions():
        if os.path.exists("{}/__init__.py".format(os.path.join(basedir, x)))
    ]
    # Place ``runestone.common`` first, so it can run init code needed by all other modules. This assumes that the first module in the list is run first. An alternative to this to guarantee this ordering is to call ``app.setup_extension('runestone.common')`` in every extension.
+    modules.insert(0, modules.pop(modules.index('runestone.common')))


This is duplicated -- see line 78.

bjones1 · 2019-11-04T18:00:17Z

runestone/assess/assess.py

@@ -32,11 +32,12 @@ def setup(app):
    app.add_directive("qnum", QuestionNumber)
    app.add_directive("timed", TimedDirective)

-    app.add_config_value("mchoice_div_class", "runestone alert alert-warning", "html")
+    app.add_config_value('mchoice_div_class', 'runestone alert alert-warning', 'html')


There should be left unchanged -- it looks like code that black processed in the original vs. no-formatted code in the diff.

bjones1 · 2019-11-04T18:02:56Z

runestone/fitb/fitb.py

-        FITBFeedbackNode, html=(visit_fitb_feedback_node, depart_fitb_feedback_node)
-    )
-    app.add_config_value("fitb_div_class", "runestone", "html")
+


Revert all but line 42.

mostly code restructure

bjones1

This looks good to me! Thanks for your contribution.

nenadmilutinovic and others added 2 commits October 16, 2019 16:12

added config for compare button

0b1a55a

updated docstrings for MChoice and FillInTheBlank

ad5bb1d

maric993 force-pushed the nm/V2CompatibilityUpdate branch from 84566c3 to ad5bb1d Compare October 17, 2019 08:31

maric993 added 4 commits October 24, 2019 17:09

changed deprecated chrome_options to options and fitb multiplechoice fix

ae91929

new updateconfig directive and tests

7ab3f18

added .vs to git

5d8ae5d

made updateConfig available only in test dirs

750ef5a

bnmnetp requested a review from bjones1 October 28, 2019 13:30

bjones1 suggested changes Oct 31, 2019

View reviewed changes

Code review fixes

789815e

updates to: - testing element presence - comment wording and implementation - general code cleanup

bjones1 suggested changes Nov 1, 2019

View reviewed changes

maric993 added 2 commits November 2, 2019 09:41

follow up fixes

90d33ed

added doc for update config

203de5d

Merge branch 'master' into nm/V2CompatibilityUpdate

eed1f01

bjones1 suggested changes Nov 4, 2019

View reviewed changes

quick fix

ac5b251

mostly code restructure

bjones1 approved these changes Nov 5, 2019

View reviewed changes

More config values for Runcstone components #867

Are you sure you want to change the base?

More config values for Runcstone components #867

Conversation

vasiljevic commented Aug 4, 2019

bnmnetp commented Aug 4, 2019

vasiljevic commented Aug 4, 2019

bjones1 commented Aug 7, 2019

bnmnetp commented Aug 7, 2019

bnmnetp commented Aug 7, 2019

vasiljevic commented Aug 8, 2019

bnmnetp commented Aug 8, 2019

bjones1 commented Aug 9, 2019

bnmnetp commented Sep 17, 2019

vasiljevic commented Oct 14, 2019

bnmnetp commented Oct 28, 2019

bjones1 commented Oct 28, 2019

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

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

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

maric993 commented Nov 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjones1 left a comment

Choose a reason for hiding this comment