First time setup fixes #3911

deivid-rodriguez · 2016-02-04T15:34:22Z

Hi friends!

This PR is just an excuse to say hi! I've loved gratipay since a long time ago and really enjoy your way of building things. And I want to offer some help! These are the changes I made to get the app up and running and the tests passing. I must say I cheated a bit...

I also wanted to ask for some beginner pending tasks for someone who knows nothing about coding in python, but wants to learn. :)

* Using `shell` to hint markdown on syntax highlighting. * Don't use `$` before commands. It makes copy-pasting much harder. * Normalize whitespace.

In order to make it clearer which steps are optional and which steps are not.

chadwhitacre · 2016-02-04T15:52:32Z

Yay! Thanks and welcome, @deivid-rodriguez! I've made an onboarding ticket for you at gratipay/inside.gratipay.com#489. Feel free to use that to ask any random questions you have about Gratipay and how to contribute. :)

deivid-rodriguez · 2016-02-04T18:16:37Z

Thanks @whit537 ! I'll ask there any questions I may have!

chadwhitacre · 2016-02-04T18:26:16Z

Re: 89e037d

I was getting an error in the tests in test_authed_hompage.js because my default browser language is Spanish, so the texts don't match.

Ah, interesting! Makes sense. :)

I started trying to fix it, but at some point I read the test and wondered whether these tests are actually testing any specific js functionality. Are they?

Barely. We do much more Python testing than JavaScript. We have a ticket to adopt a different js testing framework that would hopefully make it easier for us to write js tests: #3684. I think I'm okay with dropping the js tests for now, though I'd feel better if we ported them to #3684 instead of dropping them entirely. Opinions, @rohitpaulk @aandis et al.?

chadwhitacre · 2016-02-04T18:29:02Z

Re: 9755970

I think this files should be gitignored

Perhaps. On the other hand, they're supposed to be removed by the test suite when it completes successfully. If they're still around it can cause problems—like if the test suite crashes, the files are still around, and then you hit http://localhost:8537/ and wonder why the heck your CSS/JS changes aren't showing up. It's because they're masked by the generated files from the test suite. I think @clone1018 got bit by this once?

If the files are not git ignored then it's easier to notice them so you can clean up manually.

chadwhitacre · 2016-02-04T18:30:29Z

The rest of your commits look great. :)

deivid-rodriguez · 2016-02-04T18:37:04Z

Perhaps. On the other hand, they're supposed to be removed by the test suite when it completes successfully. If they're still around it can cause problems—like if the test suite crashes, the files are still around, and then you hit http://localhost:8537/ and wonder why the heck your CSS/JS changes aren't showing up. It's because they're masked by the generated files from the test suite. I think @clone1018 got bit by this once?

If the files are not git ignored then it's easier to notice them so you can clean up manually.

Makes sense, I didn't know they were supposed to be removed. The test suite was crashing initially for me, because I got bitten by this. While working on figuring out what was the issue, I noticed those files and thought they were always supposed to be there, so I gitignored them. So I guess an improvement instead would be to get those removed even when the test suite errors. In any case, let me remove the commit! 😉

chadwhitacre · 2016-02-04T18:42:41Z

So I guess an improvement instead would be to get those removed even when the test suite errors.

👍, though to be honest I'm not sure why we have them in the first place. Our dynamic CSS and JS files are behind a CDN in production, and I suspect that pregenerating them at all is an over-optimization (adding complexity, you see ;).

deivid-rodriguez · 2016-02-04T18:50:16Z

Is it here where those optimizations landed? Was there another reason?

chadwhitacre · 2016-02-04T18:51:09Z

No, further back ...

chadwhitacre · 2016-02-04T18:52:46Z

Here's where the files get written ...

chadwhitacre · 2016-02-04T18:54:39Z

Is it here where those optimizations landed?

#2838

removed even when the test suite errors

See #3155 for a previous attempt at this.

chadwhitacre · 2016-02-04T18:57:53Z

Was there another reason?

Looks like we didn't really answer the "why" question. The compile_assets function was part of a larger PR.

chadwhitacre · 2016-02-04T18:59:17Z

Don't ignore assets/gratipay.{css,js} after all

8ed11d9 ;-)

chadwhitacre · 2016-02-04T18:59:48Z

why ignore the compiled assets in git ? then you can't see in git status that you have stray files

Confirmed.

deivid-rodriguez · 2016-02-04T19:00:34Z

Thanks @whit537, lots of helful links and comments!

What I'll do is removing 89e037d and then go work on #3684 since it will probably help with most of the other issues mentioned here. Sounds good?

chadwhitacre · 2016-02-04T19:01:38Z

Sounds good?

Sounds good! :-)

First time setup fixes

chadwhitacre · 2016-02-04T19:07:09Z

Thanks for the cleanup, @deivid-rodriguez! :-)

Changaco · 2016-02-04T19:45:05Z

Looks like we didn't really answer the "why" question. The compile_assets function was part of a larger PR.

I don't really remember, but looking back at that PR it seems to me that compiling was necessary. How do you properly cache by hash if you don't have a compiled file to hash?

Our dynamic CSS and JS files are behind a CDN in production, and I suspect that pregenerating them at all is an over-optimization (adding complexity, you see ;).

CDNs aren't infallible, and neither is the Gratipay app, a bug in either one could result in more requests for assets.

chadwhitacre · 2016-02-04T19:51:57Z

How do you properly cache by hash if you don't have a compiled file to hash?

Yeah, but we don't necessarily need to store the compiled version, since we can assume that a recomputed version will be identical (because we trust that neither the inputs nor the computation will have varied). It might take some gymnastics to do this though, depending on how the caching and the hashing are implemented—and we have bigger 🐟 to fry. :-)

Changaco · 2016-02-04T22:33:29Z

since we can assume that a recomputed version will be identical

No, you can't, you can't even check that the inputs haven't changed because you don't know what the inputs are.

I think you underestimate the elegance of the assets compiler, it's less than 40 lines of code, it just works, and for any new .spt file under /assets too without any configuration required. :-)

Anyway, we both have bigger fish to fry, as you said.

techtonik · 2016-02-05T11:53:42Z

My wish for asset compiler is to be able to compile them as a separate self-sufficient step before running application. This is needed for transition from one CSS compiler to another. It would be nice to have something like Django's manage.py in the root that can do:

gratipay serve
gratipay compile

chadwhitacre · 2016-02-05T13:35:20Z

I think you underestimate the elegance of the assets compiler

:-)

!m @Changaco

My wish for asset compiler is to be able to compile them as a separate self-sufficient step before running application.

That doesn't sound right to me, but we can discuss in another ticket.

David Rodríguez added 3 commits February 4, 2016 15:27

README normalization

8ad608f

* Using `shell` to hint markdown on syntax highlighting. * Don't use `$` before commands. It makes copy-pasting much harder. * Normalize whitespace.

Minor rewording

e0e6b31

In order to make it clearer which steps are optional and which steps are not.

Fix first time setup

0353834

deivid-rodriguez force-pushed the first_time_setup_fixes branch from b17580f to 0ff9b99 Compare February 4, 2016 15:50

chadwhitacre mentioned this pull request Feb 4, 2016

onboard @deivid-rodriguez gratipay/inside.gratipay.com#489

Closed

deivid-rodriguez force-pushed the first_time_setup_fixes branch from 0ff9b99 to 5880819 Compare February 4, 2016 15:51

deivid-rodriguez force-pushed the first_time_setup_fixes branch from 5880819 to a0b8333 Compare February 4, 2016 16:20

deivid-rodriguez force-pushed the first_time_setup_fixes branch from a0b8333 to fee739b Compare February 4, 2016 18:37

chadwhitacre mentioned this pull request Feb 4, 2016

jstests fail when Gratipay is already running #2953

Closed

Fix title and anchor of a section in README

9e46311

deivid-rodriguez force-pushed the first_time_setup_fixes branch from fee739b to 9e46311 Compare February 4, 2016 19:02

chadwhitacre added a commit that referenced this pull request Feb 4, 2016

Merge pull request #3911 from deivid-rodriguez/first_time_setup_fixes

3bd1049

First time setup fixes

chadwhitacre merged commit 3bd1049 into gratipay:master Feb 4, 2016

Changaco mentioned this pull request Feb 5, 2016

Improve response times liberapay/liberapay.com#115

Closed

2 tasks

chadwhitacre mentioned this pull request Aug 31, 2017

Stop compiling assets #4592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First time setup fixes #3911

First time setup fixes #3911

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

Changaco commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

Changaco commented Feb 4, 2016

techtonik commented Feb 5, 2016

chadwhitacre commented Feb 5, 2016

First time setup fixes #3911

First time setup fixes #3911

Conversation

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

deivid-rodriguez commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

Changaco commented Feb 4, 2016

chadwhitacre commented Feb 4, 2016

Changaco commented Feb 4, 2016

techtonik commented Feb 5, 2016

chadwhitacre commented Feb 5, 2016