Add contribution guide #527 #528

fflorent · 2023-06-06T07:14:36Z

This PR fixes issue #527:

This adds a general contribution guide as suggested by the community Github documentation;
This helps developers to bootstrap setup their environment and start contributing;

Feedback highly welcome :)

paulfitz

Thanks @fflorent, this will be helpful! Could do with linking to it from the README, you have more precise information about building than it does.

paulfitz · 2023-06-08T02:33:38Z

.github/CONTRIBUTING.md

+- [translate](/documentation/translate.md)
+- [write tutorials and user documentation](https://github.com/gristlabs/grist-help)
+- [develop](/documentation/develop.md)
+- report issues or suggest enhancement (TODO)


TODO would be something like https://github.com/gristlabs/grist-core/issues/new?

Probably.

Maybe to go further, an issue template could help ^^.

documentation/develop.md

paulfitz · 2023-06-08T02:44:13Z

documentation/develop.md

+
+To setup your environment, you would need to install the following dependencies:
+ - git
+ - [nvm](https://github.com/nvm-sh/nvm/blob/master/README.md)


I use nvm personally but not sure if I'd put it as a requirement; could be a recommendation. The requirement is some version of node. The exact version to recommend is a bit fluid, I think I'd say 16 (though I think our tests and docker image may still use an older version), last I heard there was some awkwardness running browser tests with node 18 on osx m1 (cc @georgegevoian).

I think 18 may work as long as it's not the arm64 version. The wrinkle with tests was that the version of chromedriver we currently use is stale (74) and doesn't support arm64, but I think when I tested a while back with newer versions most tests still passed, and the few that didn't were due to breaking changes in Selenium.

paulfitz · 2023-06-08T02:45:58Z

documentation/develop.md

+To setup your environment, you would need to install the following dependencies:
+ - git
+ - [nvm](https://github.com/nvm-sh/nvm/blob/master/README.md)
+ - Firefox and/or Chromium to run the end-to-end (nbrowsers) tests


Do you know if the tests can actually run with firefox? We used to test with both, but then slipped to just using one (chrome).

I think so, I'll test that.

And maybe just say "browser tests" instead of "(nbrowsers) tests".

Don't nbrowser tests cover not only the browser but also the back-end? I would keep the term end-to-end tests, unless all of them don't require to run a server.

Ok, end-to-end is fine. I was hoping to replace/eliminate nbrowsers since it was a bit jargony (user won't know anything about the idiosyncratically named nbrowser tests yet) and also the s is maybe a bit confusing (the test target it is referring to is nbrowser). How about just dropping (nbrowsers)?

Ah, I see it is already fixed 👍

Ok, end-to-end is fine

If you're uncomfortable with naming them end-to-end, don't hesitate to tell me. I am not very confident on how to name them ^^

paulfitz · 2023-06-08T02:51:21Z

documentation/develop.md

+$ npm install -g yarn
+```
+
+Now each time you want to load nodejs and yarn in your environment, just run the following command at grist-core root directory:


"at grist-core" -> "in the grist-core"

documentation/develop.md

paulfitz · 2023-06-08T03:00:45Z

documentation/develop.md

+
+## Develop widgets
+
+TODO


If this is about custom widgets, it could point to https://github.com/gristlabs/grist-widget#readme or https://support.getgrist.com/widget-custom/ (though I do see looking at this now that neither is ideal).

fflorent

Thanks for your review @paulfitz, I changed some stuffs in accordance to your remarks.

documentation/develop.md

fflorent · 2023-06-08T16:21:32Z

documentation/develop.md

+To setup your environment, you would need to install the following dependencies:
+ - git
+ - [nvm](https://github.com/nvm-sh/nvm/blob/master/README.md)
+ - Firefox and/or Chromium to run the end-to-end (nbrowsers) tests


I think so, I'll test that.

documentation/develop.md

paulfitz

thanks for iterating, just a few small comments

paulfitz · 2023-06-08T20:40:50Z

documentation/develop.md

+
+#### Using nodejs
+
+You can also use nodejs installed in your system. To prevent instabilities, ensure that the `node --version` command a version equal or ulterior to the one in `.nvmrc`.


instabilities -> incompatibilities?
"command a version" -> "command reports a version"
I'd say "greater" rather than "ulterior"

I'd still vote against ulterior. Maybe "an equal or later/greater version"?

Oopsi! I fix that, sorry for having missed that one.

documentation/develop.md

paulfitz

Almost there! Thanks for doing this (and for your patience).

documentation/develop.md

paulfitz · 2023-06-13T16:26:19Z

documentation/develop.md

+To setup your environment, you would need to install the following dependencies:
+ - git
+ - [nvm](https://github.com/nvm-sh/nvm/blob/master/README.md)
+ - Firefox and/or Chromium to run the end-to-end (nbrowsers) tests


Ah, I see it is already fixed 👍

documentation/develop.md

paulfitz

Thanks @fflorent !

fflorent · 2023-06-14T12:05:21Z

@paulfitz Good! 😄

I rebased and squashed the commits for more clarity before you merge it

paulfitz

Thanks @fflorent !

Co-authored-by: Florent <florent.fayolle@beta.gouv.fr>

paulfitz reviewed Jun 8, 2023

View reviewed changes

fflorent force-pushed the issue-527-contribution-guide branch from 6884955 to d22c94d Compare June 8, 2023 16:25

fflorent commented Jun 8, 2023

View reviewed changes

fflorent mentioned this pull request Jun 8, 2023

Add contribution guide gristlabs/grist-widget#48

Open

paulfitz reviewed Jun 8, 2023

View reviewed changes

documentation/develop.md Outdated Show resolved Hide resolved

paulfitz reviewed Jun 8, 2023

View reviewed changes

fflorent requested a review from paulfitz June 13, 2023 08:15

paulfitz reviewed Jun 13, 2023

View reviewed changes

paulfitz approved these changes Jun 13, 2023

View reviewed changes

Add contribution guide gristlabs#527

5f3a0c5

fflorent force-pushed the issue-527-contribution-guide branch from b4df9c4 to 5f3a0c5 Compare June 14, 2023 12:02

paulfitz approved these changes Jun 14, 2023

View reviewed changes

paulfitz merged commit 6a2010f into gristlabs:main Jun 14, 2023

fflorent mentioned this pull request Jun 20, 2023

Add a contribution guide #527

Closed

fflorent added a commit to incubateur-territoires/grist-core that referenced this pull request Jun 20, 2023

Add contribution guide gristlabs#527 (gristlabs#528)

b9ddaed

Co-authored-by: Florent <florent.fayolle@beta.gouv.fr>

vviers added the anct label Jul 11, 2023

fflorent deleted the issue-527-contribution-guide branch July 11, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contribution guide #527 #528

Add contribution guide #527 #528

fflorent commented Jun 6, 2023 •

edited

Loading

paulfitz left a comment

paulfitz Jun 8, 2023

fflorent Jun 8, 2023

paulfitz Jun 8, 2023

georgegevoian Jun 8, 2023

paulfitz Jun 8, 2023

fflorent Jun 8, 2023

paulfitz Jun 8, 2023

fflorent Jun 13, 2023

paulfitz Jun 13, 2023

paulfitz Jun 13, 2023

fflorent Jun 13, 2023

paulfitz Jun 8, 2023

paulfitz Jun 8, 2023

fflorent left a comment

fflorent Jun 8, 2023

paulfitz left a comment

paulfitz Jun 8, 2023

paulfitz Jun 13, 2023

fflorent Jun 13, 2023

paulfitz left a comment

paulfitz Jun 13, 2023

paulfitz left a comment

fflorent commented Jun 14, 2023

paulfitz left a comment


		#### Using nodejs

		You can also use nodejs installed in your system. To prevent instabilities, ensure that the `node --version` command a version equal or ulterior to the one in `.nvmrc`.

Add contribution guide #527 #528

Add contribution guide #527 #528

Conversation

fflorent commented Jun 6, 2023 • edited Loading

paulfitz left a 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

fflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz left a 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

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz left a comment

Choose a reason for hiding this comment

fflorent commented Jun 14, 2023

paulfitz left a comment

Choose a reason for hiding this comment

fflorent commented Jun 6, 2023 •

edited

Loading