RFC: Consistent scripts to rule them all #309
Conversation
Proposal for adopting and adapting a consistent set of scripts across our OSS repositories: > Being able to get from git clone to an up-and-running project in a development environment is imperative for fast, reliable contributions.
See PostHog/meta#309 for the rationale behind these scripts.
See PostHog/meta#309 for context.
See PostHog/meta#309 for con 8000 text.
|
|
||
| ## Solution | ||
|
|
||
| Adopt and adapt GitHub's approach of [scripts to rule them all](https://github.blog/engineering/engineering-principles/scripts-to-rule-them-all/) to |
There was a problem hiding this comment.
TIL github approach for scripts
| My recommendation is to still standardize on `bin` scripts so that it's consistent across the board. Some of the bin scripts are pretty complicated and it'll be hard to port to `make`. | ||
| If some repos have `make` files, that's fine. The bin scripts can call the `make` commands. |
There was a problem hiding this comment.
the only downside here is that we have a bit of duplication (which is fine since only Mobile has the make files anyway)
| * `bin/build` - Build the project, for projects that are compiled such as C#, Java, etc. | ||
| * `bin/start` - Start the project. For SDKs, this might start an example server. | ||
| * `bin/test` - Run tests (Ex. `npm test`, `bundle exec rspec`, etc.). Also includes linting, formatting, etc. | ||
| * `bin/fmt` - Optional: Format/lint code. This can be called by `test`. |
There was a problem hiding this comment.
I think fmt and lint should be different tasks
There was a problem hiding this comment.
Maybe lint should just go into the bin/build task? Hear me out.
It seems to me that linting for a dynamic language serves a similar purpose to compilation in a compiled language in that it points out obvious errors.
So for a language like python, bin/build could run both mypy and flake8. Thoughts?
There was a problem hiding this comment.
I'm open to just having it be a separate script as well.
There was a problem hiding this comment.
I think for speedy, i prefer the lint to be its own thing, but it really depends on the tooling.
what i did in the past and worked really well is that i have a cmd for each step (fmt, lint, etc), and a cmd that does it all (eg build) that will run all its dependencies (fmt lint, etc).
eg https://github.com/PostHog/posthog-android/blob/9906ac8ac333a0e4990b64dceeca892453d81303/Makefile#L6-L7
^ runs everything else, so in CI I just do that, but locally if i want to check something, i can just make format
There was a problem hiding this comment.
I like that! I think what this "spec" will define is a recommended minimum set of scripts. So if you only have build, it should do all these things. But you can optionally break down build into multiple steps, each with their own script. That's a pretty good pattern for this.
|
|
||
| * What other scripts do we need? | ||
| * What should the scripts be named? | ||
| * Should we use `bin` or something else? |
There was a problem hiding this comment.
I'd rather use just scripts, bin is used by some other languages/frameworks (eg a dart project will have its executable and other entry points in the bin folder) as part of the folder structure and our scripts will be among many different files, a bit confusing
There was a problem hiding this comment.
I'll let you and @rafaeelaudibert debate bin vs scripts. 😆
There was a problem hiding this comment.
I vouched for bin because we're already using and I don't wanna hurt our muscle memory.
It is true bin is used in stuff like Ruby/Rails as well to include default scripts, but isn't this just the same as we already have here? The only difference is that they might have some extra files inside bin that don't exist in Django/JS land, for example
If that's not the case, and they're more than just scripts, I'm fine changing to scripts. it will hurt in the short-term, but it doesnt really matter in the long-term
There was a problem hiding this comment.
Some languages/frameworks have bin as part of .gitignore btw, you'd need to remove from gitignore and add all the other files that should not be checked under the source control.
no strong opinions here, I just think that scripts would be cleaner and avoid this known pitfall.
for muscle memory, you could either set up your own alias or even have a symbolic file linked to the correct one so if you do eg ./lint in the root folder, it just works it does not matter if its within the bin or scripts really
|
nice, thanks for the initiative @haacked |
| * `script/cibuild` - used by the CI system to run tests, build, etc. | ||
| * `script/console` - is used to open a console for your application. | ||
|
|
||
| ## Design |
There was a problem hiding this comment.
Love the idea!
I'd love to also standardise the release process for new versions of a library, I don't think you need to do that in this PR though
There was a problem hiding this comment.
I'd love to also standardise the release process for new versions of a library, I don't think you need to do that in this PR though
Same! It's on my list of things to do. With RELEASING, I think I'll document the many ways we do it today and propose a couple ideas for how we can consolidate it into a common approach. Obviously, not all things can be released in exactly the same way, but we can probably come up with a framework for releasing things that's fairly consistent.
There was a problem hiding this comment.
You can inspire from (or even use it) https://github.com/getsentry/craft
We built craft at sentry exactly for this reason
There was a problem hiding this comment.
I'd also recommend having the release and contributing markdown in the repos
eg https://github.com/PostHog/posthog-android/blob/main/RELEASING.md
https://github.com/PostHog/posthog-android/blob/main/CONTRIBUTING.md
so that open source contributors (and new employees) know how to build, run, and release things.
* Add bin scripts See PostHog/meta#309 for context. * Reimplement `isfeatureEnabled` using `getFeatureFlag` Changes `isFeatureEnabled` to delegate to `getFeatureFlag` in order to get the result. This ensures that the `$feature_flag_called` event sends the correct `$feature_flag_response`. Fixes #336 * Add changelog entry * Remove unused method and update tests * Update date in CHANGELOG.md Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> --------- Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
Proposal for adopting and adapting a consistent set of scripts across our OSS repositories: