update CONTRIBUTING.md to support local project build

[jshreyans]

Checkout #203

Problem

The contributing doc did not have clear instructions for first-time contributors to build and run the project locally. Also, the present instructions require the user to have a local testmine running on their machine, which they might find confusing at first.

Solution

As suggested by @uosl, a root html file has been added in #204 to serve im-tables with Flymine. This PR updates the documentation according to these changes.

[jshreyans]

For this PR could you modifying it so a changed version of imjs and imtables are not checked in (since this is only a docs change, not a functionality change)?

Could you elaborate on this, please?

[yochannah]

@jshreyans if you look at the files changed tab for this PR it shows three modified files - CONTRIBUTING.md, dist/imtables.js, and dist/imtables.min.js (ignore my comment about imjs, whoops). This PR is documentation-only, so it would be better if it contained only CONTRIBUTING.md and no other changed files.

[jshreyans]

Ah got you now. I'll have a look. But I wanted to point this out too. Whenever I build the project or install/update an npm package, the dist folder changes by itself. The 2 files you mentioned specifically. This is weird behavior and keeps coming up in every change. In fact, @uosl had to commit changes to the dist folder in #202 after I made the PR. Any idea why this is happening?

[yochannah]

It's mostly because this package was made when node was still very young (node 0.x) and some of the current norms regarding packaging and scripting weren't around yet. The developer who built this package designed it to run the tests and build after every single change. We could probably tidy things up a little in the /bin and package.json and grunt scripts to make it match more modern behaviours, but it's not urgent right now I think.

[yochannah]

(as a side note - it used to be that to even build im-tables you needed to have a full testmine running (or the build would fail on tests) - things are better than they were a couple of years ago 😆

[jshreyans]

So to fix this, one thing I can do is remove my commit so that changes to dist are reverted and then just change the contributing doc and commit the file again. Is that fine?

[yochannah]

perfect!

On Mon, 10 Feb 2020 at 16:05, Shreyans Jain notifications@github.com wrote:

So to fix this, one thing I can do is remove my commit so that changes to dist are reverted and then just change the contributing doc and commit just file again. Is that fine?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/intermine/im-tables/pull/206?email_source=notifications&email_token=ACGXRDW4XPH62ZXTFQEER53RCF3NDA5CNFSM4KSAPWB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELJCLOI#issuecomment-584197561, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGXRDQF4BWV5LBLWQ74OKLRCF3NDANCNFSM4KSAPWBQ .

-- Please note I am ‘working to contract’ as part of the UCU industrial action to defend our pensions and fight for fair pay and equality. This may mean it takes longer for me to respond to emails. You can find out more about the dispute at: www.ucu.cam.ac.uk/faqs http://www.ucu.cam.ac.uk/faqs

[heralden]

The package.json has a postinstall script (all it does is grunt build) which is a kind of hook for npm that is activated after every npm install command. I think it makes sense since it's a common mistake to change a dependency and then forget to commit the new bundle.

The weird thing is that the bundle changes on your computer, but not on mine (I just tried to run npm install and have it rebuild the bundle, and no changes). Maybe it's because I'm on Linux and you're on a Mac? (@yochannah could test to confirm.)

[jshreyans]

@uosl I'm running Linux myself (Ubuntu 18.04). If this is specific to my machine, I'll have to look for a solution because this becomes a problem when I make any change to some file and commit that change.

[jshreyans]

@yochannah I've reverted the changes to the files in the dist folder. Although it still reflects in the files changed section which is weird. Also, after #209, the doc had to be modified again.

The thing is, running npm start will no longer require the user to have a running testmine. So this section of the docs is no longer needed where it is:

To use the test indices you will need a data server running the intermine-demo application at port 8080 on your machine - you can get this by running the testmodel/setup.sh script in the intermine/intermine repo.

How do you suggest I change this?

[heralden]

I would turn that paragraph into its own section on testing, along with a mention of the watchTests (or whatever the new command ends up being) command from #209. This is suitable since you'll need the testmodel for all the tests to pass.

[jshreyans]

Fine then I'll try and complete the work in #209 and then finish the documentation

[jshreyans]

@uosl I've made changes to contributing.md according to the latest Gruntfile changes. It's finally in sync with the current state of the project.