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

Add development/contributor documentation #391

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ascheman
Copy link
Contributor

@ascheman ascheman commented May 29, 2024

Currently, this is work-in-progress and needs some discussion by the team!

First review by @hendrikebbers required.

I recommend opening the new CONTRIBUTING.adoc in IntelliJ (with AsciiDoc and PlantUML installed), or to start with the
current version as PDF.

ascheman added 2 commits May 29, 2024 10:50
as it only covers the frontend and does not use specified Node and NPM versions.
focussing on build processes and Git hooks.
@hendrikebbers
Copy link
Member

Adding @Ndacyayisenga-droid and @Jexsie for discussion.

Based on my current knowledge I would favor the following behavior:

  • No git hooks are added by default. It is fine to have a script for that and make it possible to add the hooks locally but it should not be added "by default".
  • The NPM build should be startable independent of the Maven build (calling npm directly from frontend folder)
  • A Maven call (mvnw verify) should call the npm build, too. Here it would be perfect to have a profile that ignores all npm steps (@ascheman a good issue for future)
  • I like the setup-benchscape.sh and start-benchscape.sh scripts in the root folder. that files should stay.
  • I do not see a benefit the Makefile in addition. We should get rid of that (@Jexsie do I miss something here)

@ascheman
Copy link
Contributor Author

  • No git hooks are added by default. It is fine to have a script for that and make it possible to add the hooks locally but it should not be added "by default".

As we seem to have different types of development persons (frontend vs. backend, NPM vs. Maven) we either should find something which make everyone happy or make it optional (as proposed in the guide).

  • The NPM build should be startable independent of the Maven build (calling npm directly from frontend folder)

👍

As Maven installs the respective (POM.xml configured NPM and Node binaries) to a local path (frontend/node/node and frontend/node_modules/npm/bin/npm), we could use those versions locally as well by adding the respective directories to ${PATH}. This would ensure that the very same version is used even locally (as long as Maven was called prematurely).

But I am no frontend developer, we should handle it in a way which is most convenient to the frontend people.

  • A Maven call (mvnw verify) should call the npm build, too. Here it would be perfect to have a profile that ignores all npm steps (@ascheman a good issue for future)

This is already the case, and we could easily add a respective profile. Feel free to file an issue for that and assign it to me for future work.

  • I like the setup-benchscape.sh and start-benchscape.sh scripts in the root folder. that files should stay.

I would at least recommend to unify and simplify them. As stated in the guide, setup-benchscape.sh already builds AND starts the services. Therefore, start-benchscape.sh seems redundant (as I would think, for local development you only want to start both applications with a fresh build)?

But again, frontend developers should say what they use in their daily work and which other use cases they perhaps have?

  • I do not see a benefit the Makefile in addition. We should get rid of that (@Jexsie do I miss something here)

💯

@ascheman
Copy link
Contributor Author

If you don't mind, I would even unify the README (.md) and the new CONTRIBUTING guide (and move all developer centric docs to the latter).

@Jexsie
Copy link
Member

Jexsie commented May 30, 2024

Thanks, @ascheman and @hendrikebbers, we totally agree with the above suggestions. The main reason for the Makefile was to have something that can be easily read and remembered, we can ignore it still.

As Maven installs the respective (POM.xml configured NPM and Node binaries) to a local path (frontend/node/node and frontend/node_modules/npm/bin/npm), we could use those versions locally as well by adding the respective directories to ${PATH}. This would ensure that the very same version is used even locally (as long as Maven was called prematurely).
But I am no frontend developer, we should handle it in a way which is most convenient to the frontend people.

This sounds technically fine. I won't say I got what you meant 🙂, but yeah, sounds good to me.

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.

3 participants