Skip to content
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

Another go at updating dependencies #192

Merged
merged 10 commits into from
Oct 23, 2016
Merged

Conversation

joneshf
Copy link
Member

@joneshf joneshf commented Oct 23, 2016

Re: #160

@joneshf joneshf mentioned this pull request Oct 23, 2016
@@ -25,7 +25,7 @@ structures:
* [Bifunctor](#bifunctor)
* [Profunctor](#profunctor)

<img src="figures/dependencies.png" width="677" height="212" />
<img src="figures/dependencies.png" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use the svg rather than the png here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just kill the png altogether.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. See my Stack Overflow answer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpominov added the dimensions in #119 for a good reason. We should specify the new dimensions to avoid regressing.

@@ -0,0 +1,35 @@
digraph {
node [fontcolor=blue;shape=plaintext]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's s/blue/#4078c0/ to match GitHub's link colour.

@@ -25,7 +25,7 @@ structures:
* [Bifunctor](#bifunctor)
* [Profunctor](#profunctor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this list. It's duplicative now that the diagram is up to date. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to keep this. It's nice to have a fallback in case something happens with the image, different people process information differently, and it's still plaintext so it can be searched.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two points in favour of the ASCII version. 😜

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

I'm fine with the output graph being ASCII. I just don't want to maintain the shape of the graph in raw ASCII.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally with you. A program like dot which generates an ASCII diagram would be neat!

I very much like your change. Thanks for taking the time to work on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does support multiple output formats, including plain. plain gives you the size of the graph, a sequence of nodes with positions, and a sequence of edges with position if you want to parse the coordinates into an ascii thing. It looks like a regular language, so probably not too hard to parse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I may investigate that one day (but certainly not today).

@@ -25,7 +25,7 @@ structures:
* [Bifunctor](#bifunctor)
* [Profunctor](#profunctor)

<img src="figures/dependencies.png" width="677" height="212" />
<img src="https://cdn.rawgit.com/fantasyland/fantasy-land/master/figures/dependencies.svg" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inadvisable:

Use a specific tag or commit hash in the URL (not a branch). Files are cached permanently based on the URL.

The approach I used with Orthogonal was to add the SVG in one commit then reference its location by commit hash in the following commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Makes sense.

@davidchambers
Copy link
Member

@joneshf, what command are you running to generate dependencies.svg? Could you add it to a makefile so the rest of us know how to do it?

@joneshf
Copy link
Member Author

joneshf commented Oct 23, 2016

This file: https://github.com/fantasyland/fantasy-land/blob/4c1e3ee239c9a68e8d0a9a299b663e48d4cb6cfe/figures/dependencies.sh

@davidchambers
Copy link
Member

The links in the image do not work, because GitHub wraps the image in an anchor element. Shall we remove the links? Since we're going to keep the list of section links they're not essential.

@joneshf
Copy link
Member Author

joneshf commented Oct 23, 2016

Sure.

@davidchambers
Copy link
Member

It's looking good! Ideally we'd now have two clean commits:

Commit 1:

  • add figures/dependencies.dot
  • add figures/dependencies.sh
  • update figures/dependencies.svg

Commit 2:

  • update README.md to point to RawGit SVG (include hash of Commit 1)
  • remove figures/dependencies.mp
  • remove figures/dependencies.png

I'm happy to do this if fiddling with git rebase is not the way you like to spend your Sundays. :)

⚠️ We'll then merge the pull request without using the "squash and merge" feature.

@joneshf
Copy link
Member Author

joneshf commented Oct 23, 2016

Nah, rebase is fine.

I wonder if svg is worth it if we have to use rawgit like this. Remembering to make one commit to change the graph and one to update the sha is less than ideal. If we use png rather than svg for output, it seems like we could do it in one commit, and similar for future changes. What do you think?

@davidchambers
Copy link
Member

Good point. Let's stick with a PNG. :)

@davidchambers
Copy link
Member

Excellent! If you're happy with the current state of the diff, Hardy, let's squash and merge. :)

@joneshf joneshf merged commit 2a94dce into fantasyland:master Oct 23, 2016
@joneshf joneshf deleted the dependencies branch October 23, 2016 14:34
@SimonRichardson
Copy link
Member

👍

rjmk added a commit to rjmk/fantasy-land that referenced this pull request Nov 12, 2016
* 'master' of github.com:fantasyland/fantasy-land: (29 commits)
  Version 2.1.0
  Add Alt, Plus and Alternative specs (fantasyland#197)
  Use uppercase letters for Type representatives in laws (fantasyland#196)
  Fix id_test and argument order in laws (fantasyland#193)
  Version 2.0.0
  Another go at updating dependencies (fantasyland#192)
  release: integrate xyz (fantasyland#191)
  test: remove unnecessary lambdas (fantasyland#190)
  require static methods to be defined on type representatives (fantasyland#180)
  lint: integrate ESLint (fantasyland#189)
  Enforce parametricity (fantasyland#184)
  readme: tweak signatures to indicate that methods are not curried (fantasyland#183)
  Fix reduce signature to not use currying (fantasyland#182)
  Link to dependent specifications (fantasyland#178)
  Add Fluture to the list of implementations (fantasyland#175)
  laws/functor: fix composition (fantasyland#173)
  laws/monad: fix leftIdentity (fantasyland#171)
  Minor version bump
  bower: add bower.json (fantasyland#159)
  Fix chainRec signature to not use currying (fantasyland#167)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants