cheatsheets icon indicating copy to clipboard operation
cheatsheets copied to clipboard

refactor handout-tips scripts

Open jimustafa opened this issue 3 years ago • 7 comments

  • combine all the handout-tips scripts into one
  • expected to help with #88

jimustafa avatar Nov 18 '21 08:11 jimustafa

This pull request introduces 3 alerts and fixes 5 when merging fa241e27a33c02c1536ac0f1539e2b76cd3f56a1 into 5f45dcbd849c5bc00b0cce95b818485d1ac44961 - view on LGTM.com

new alerts:

  • 3 for Variable defined multiple times

fixed alerts:

  • 4 for Unused import
  • 1 for Variable defined multiple times

lgtm-com[bot] avatar Nov 18 '21 08:11 lgtm-com[bot]

This pull request fixes 5 alerts when merging 7224cc5e923bd24c6c90654675c34f34376a7e1f into 5f45dcbd849c5bc00b0cce95b818485d1ac44961 - view on LGTM.com

fixed alerts:

  • 4 for Unused import
  • 1 for Variable defined multiple times

lgtm-com[bot] avatar Nov 19 '21 02:11 lgtm-com[bot]

I prefer to have one file / one idea because I find difficult some time to get the code corresponding to an image when there are several examples in the same file (e.g. gallery). Furthermore, what do we gain by having a single file?

rougier avatar Nov 22 '21 17:11 rougier

I don't have a strong opinion either way. Out of curiosity: what makes multiple files more manageable? The main obstacle is finding the appropriate name for a figure. Whether I then open [name].py or search for "[name].pdf" (from the savefig command) in a single file is no big difference.

timhoffm avatar Nov 22 '21 18:11 timhoffm

Look for example https://matplotlib.org/2.0.2/examples/statistics/boxplot_demo.html. Source is compact but as a user, if you're interested in a specific subfigure, it takes your some time to identify the code. Having one script / figure make it easier to get the relevant code.

rougier avatar Nov 22 '21 18:11 rougier

Admittedly, consolidating the scripts is probably just a matter of taste. There may be some benefit in being able to more easily recognize code patterns and keeping things DRY. Could also be easier to keep a consistent style with a single file. Either way, not a big deal.

jimustafa avatar Nov 23 '21 01:11 jimustafa

This pull request fixes 5 alerts when merging e204e249760800ee0a8db9facead2511f3eb307d into c26b5c4c058ed5048b71f3fc841332dd81d68ee4 - view on LGTM.com

fixed alerts:

  • 4 for Unused import
  • 1 for Variable defined multiple times

lgtm-com[bot] avatar Nov 23 '21 07:11 lgtm-com[bot]

Again, I don't see the advantage of having a single script. I think it is easier to have well named isolated scripts that make things easier to find for the user.

rougier avatar Mar 29 '23 18:03 rougier

Again, I don't see the advantage of having a single script. I think it is easier to have well named isolated scripts that make things easier to find for the user.

I get it. Was not sure a decision was made on this PR, so I just rebased so that it could be merged. Combining the scripts could help reduce repetition, especially as we add more per-script configuration, as in #127 and #129.

Anyways, I will close for now, and can reopen if opinion changes. Thanks!

jimustafa avatar Mar 30 '23 00:03 jimustafa