-
Notifications
You must be signed in to change notification settings - Fork 2
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
LTB-1 LTB-3 Basic project structure #1
Conversation
docs/workflow.md
Outdated
Working on an issue | ||
-------------------- | ||
|
||
1. Starting from `master` create a new _topic branch_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you decided to go away from putting branches into separate folders? (e.g. <your-nickname>/<branch-name>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the open question about cabal
. I think that we would like to use it but with hpack
it becomes more complicated to deal with it as we won't even include .cabal
files in the directory, and won't be able to change them manually cause if we do then hpack
will yell at us and won't rewrite the .cabal
. I guess at this moment we should discuss this.
Also I personally don't like that huge differences from the company's style-guide. Especially the one with indents that would be noticeably differ the code in here from all other code in the Serokell..
@@ -0,0 +1,3 @@ | |||
* Use 2 spaces for indentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? We have company style that says 4. Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this applies to YAML files, and we only specify indentation in Haskell files in our style guide. That should probably be fixed. @chshersh, care to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need separate style-guides for .yaml
and .hs
?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably that would be a good idea, because, apparently, by default Yaml.encode
renders YAML using 2 spaces (I've just tested Cardano's --dump-configuration
flag; and I don't remember having any custom rendering configuration in the code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yes, different languages call for different style guides.
Here I encoded the rules which, according to my experience, are almost universal, that is how everyone else formats YAML files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different style-guide recommendations for different languages might be ok if only this recomendations are language specific. I don't see any language specific in number of spaces for any reasonable language (both general purpose programming language and markup or configuration language).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chshersh there are language-specific conventions. For example, in Python it's 4 spaces, while in YAML it's 2. In Haskell there is no widely established convention, so we have to maintain our own.
Personally, I'm OK with going either way (accepting conventional 2-space indentation or going with 4-space in YAML).
docs/workflow.md
Outdated
--------- | ||
|
||
* The code in the `master` branch should always compile and pass all tests. | ||
* Topic branches. Naming scheme: `<issue_id>-<brief_description>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why without nickname? It's so much easier to walk through your own branches and to switch to other people's branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, nicknames makes easier to find who is responsible for this PR. Thus, it makes life easier. Often people don't press DELETE BRANCH
button which results in accumulating of outdated branches in repository. Having nickname in branch name at least allows to punish such people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is not the creator of the branch, but the person who merged the PR and did not push the delete button who needs to be punished 🤷♂️.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not about punishing, but rather who to ask whether the branch can be deleted. Also sometimes branches may not have PRs (e. g. you started working on some issue and then dropped it for whatever reason) and be forgotten for a while, but at some point we may want to do something with the branch and if it has some username, we'll know who to ask.
docs/workflow.md
Outdated
|
||
* Follow [this guide][git-commit]. | ||
* Prefix the subject line with IDs of all relevant issues. | ||
Example: `LTB-1 LTB-3 Basic project structure`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right that it should be without []
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to style guide for commit messages this should be something like
Implement basic project structure
Verb is required. Basic project structure
is not clear enough. What did you do? Fix
or Introduce
or Add
or Polish
or Delete
or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I interpreted a question as "Should issue tag be bracketed?":
LTB-1 LTB-3 Introduce basic project structure
vs [LTB-1] [LTB-3] Introduce basic project structure
🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, usage of brackets should be clarified too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vote: [LTB-1] [LTB-3] Introduce basic project structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vote: [LTB-1, LTB-3] Introduce basic project structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the intention of this point was to clarify the usage of the brackets by providing an example :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think brackets should be optional and style of brackets in commit messages is not something to restrict. The essential part is the issue id, the rest seems too minor to me.
docs/workflow.md
Outdated
4. Open a new pull request. | ||
5. Request a review from one person you think would be most suitable as a reviewer | ||
(e.g. because you think they might be interrested in your change or because | ||
they touched this code previously). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't I request it from everyone? It's not that many people in the team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least two reviews to merge.
6. If you _really_ want to, request a review from one other person. | ||
7. Change the state of the issue to “Waiting for review”. | ||
8. Review at least two other pull requests (if there are less than two open | ||
pull requests, review all that are open). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I review the PRs that are not assigned to me in case no other left? (I assume, yes) Should the reviewed PR be closed if it looks good? (I assume no, because we need everyone's acceptance to close the PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's an interesting idea. +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the first question is answered in the “reviewing” section of the document. I’ll address the seond one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I believe, the seond question is already answered below.
author: Serokell | ||
maintainer: hi@serokell.io | ||
copyright: 2018 Serokell | ||
license: MPL-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use MPL-2.0 license and not MIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short answer: it doesn’t matter anyway because it is not cleear if this project will be public.
Long answer: MPL is better than MIT for us as a company but not as bad towards others as GPL.
|
||
|
||
ghc-options: | ||
- -Wall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might find this list interesting:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I though GHC is free of this GCC "-Wall -Wextra -Wpedantic -Wreallyall -Waskingyoulasttimegivemeallthewarnings -Whatever
" nonsense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chshersh @kirelagin there is a section on GHC warnings in https://lexi-lambda.github.io/blog/2018/02/10/an-opinionated-guide-to-haskell-in-2018/ (Ctrl+F -Wall
). In particular, the author mentions -Wcompat
and -Wredundant-constraints
.
- -Wall | ||
|
||
when: | ||
- condition: os(linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed if you use GHC-8.2.2. It has gold
linker enabled by default if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I heard something about that, and I considered removing this block, but I couldn’t find a reliable proof :/.
docs/workflow.md
Outdated
--------- | ||
|
||
* The code in the `master` branch should always compile and pass all tests. | ||
* Topic branches. Naming scheme: `<issue_id>-<brief_description>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, nicknames makes easier to find who is responsible for this PR. Thus, it makes life easier. Often people don't press DELETE BRANCH
button which results in accumulating of outdated branches in repository. Having nickname in branch name at least allows to punish such people.
docs/workflow.md
Outdated
|
||
* Follow [this guide][git-commit]. | ||
* Prefix the subject line with IDs of all relevant issues. | ||
Example: `LTB-1 LTB-3 Basic project structure`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to style guide for commit messages this should be something like
Implement basic project structure
Verb is required. Basic project structure
is not clear enough. What did you do? Fix
or Introduce
or Add
or Polish
or Delete
or something else?
docs/workflow.md
Outdated
are minor and can be addressed reasonably quickly, after posting your | ||
comments you may make the required changes yourself, push and approve. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workflow guide doesn't describe resolving conflicts and merging strategies.
- Should we use
merge
orrebase
? - Should commits be squashed?
- Should developers press
Squash and rebase
button? - Should developers resolve conflicts in their branches using
merge master
orrebase onto master
? - Should all approvals be discared after pushing new changes to branch? Because, it's not enough to review PR once, you need to review again after new changes arrived and if you approved old version doesn't mean you will approve recent version.
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very-very complicated complicated topic 🙁. I’ll see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all approvals be discared after pushing new changes to branch?
We actually tried that. I eventually disabled this, I’m not sure why, but I guess the primary reason was that we were really struggling with reviews. If any of the new rules will help us getter better and more quick reviews, I think this should be enabled back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cardano-sl we have this feature enabled for important branches like master
and release/*
and disabled for develop
(which is also protected, but w/o this feature).
@@ -0,0 +1,67 @@ | |||
Branches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that we should have such workflow company-wide, not project wide. And let's all developers discuss. Personally I think that it's better if projects in company use consistent style. Some projects like cardano-sl
can have their specific guidelines because not only Serokell developers write code there. But why have different guidelines for different projects inside one company?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And furthermore, I'd insist on a strict policy where we use CI to make sure that all PRs to all projects follow the company-wide style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course, but enforcing any policy company-wide is a big decision. That’s why I’d like to run an experiment in this project, see how it works and then try to push it to the rest of the company.
snapshot.yaml
Outdated
- cborg-0.2.0.0 | ||
|
||
# for serokell-util | ||
- time-units-1.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use serokell-util >= 0.7.0
you don't need time-units
. o-clock
is preferred solution for time-safe units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
@@ -0,0 +1,8 @@ | |||
resolver: snapshot.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are advantages of snapshot.yaml
over specific resolver like lts-10.7
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least it’s cleaner.
Other than that this speeds up build by caching more build results (although I am not 100% sure whether this is true in stack 1.6 but from my limited experiments it is still true).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know about build time being decreased!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repository appears out of the blue and I don't understand how it is related to the rest of things we develop. Could you clarify its relationship to
- The company-wide style guide
universum
serokell-util
Also, please review my comments. Not all of them are actionable, but some are.
@@ -0,0 +1,67 @@ | |||
Branches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And furthermore, I'd insist on a strict policy where we use CI to make sure that all PRs to all projects follow the company-wide style guide.
@@ -0,0 +1,3 @@ | |||
* Use 2 spaces for indentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this applies to YAML files, and we only specify indentation in Haskell files in our style guide. That should probably be fixed. @chshersh, care to do that?
Commit messages | ||
---------------- | ||
|
||
* Follow [this guide][git-commit]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guide requires using verbs, btw.
docs/workflow.md
Outdated
|
||
* Follow [this guide][git-commit]. | ||
* Prefix the subject line with IDs of all relevant issues. | ||
Example: `LTB-1 LTB-3 Basic project structure`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, usage of brackets should be clarified too.
docs/workflow.md
Outdated
|
||
1. Starting from `master` create a new _topic branch_. | ||
2. Make your changes. | ||
3. Sign the most recent commit in your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, optionally, all of them. Why not do it automatically with a .gitignore
setting?
[commit]
gpgsign = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to sign all of them, it is enough to just sign the latest one, because git, hashes, that stuff. If someone wants to set gpgsign = true
and sign all commits, there is no problem with that, but there is no reason to enforce this.
docs/workflow.md
Outdated
3. Sign the most recent commit in your branch. | ||
4. Open a new pull request. | ||
5. Request a review from one person you think would be most suitable as a reviewer | ||
(e.g. because you think they might be interrested in your change or because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: interrested
6. If you _really_ want to, request a review from one other person. | ||
7. Change the state of the issue to “Waiting for review”. | ||
8. Review at least two other pull requests (if there are less than two open | ||
pull requests, review all that are open). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's an interesting idea. +1.
docs/workflow.md
Outdated
ask a question. If you are not sure what is going on, ask a questions. | ||
It is the job of the PR author to explain everything they did to everyone. | ||
It is your job to learn from the code you see and/or make sure that | ||
our repository contains only code of the highest quality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be emphasized even more. "If you don't know how this works, ask" — in bold face, framed on a wall at our office.
docs/workflow.md
Outdated
5. Request a review from one person you think would be most suitable as a reviewer | ||
(e.g. because you think they might be interrested in your change or because | ||
they touched this code previously). | ||
6. If you _really_ want to, request a review from one other person. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe requesting reviews from two people should be discouraged. In fact, I think two should be the default, and three or more would apply to complex multi-thousand lines PRs. That's the way it works in Cardano, where we request one reviewer mostly for very simple PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal here is to make sure people are not discouraged by seing that their review was not requested. It is assumed that everyone reviews everything in any case, so there is simply no reason to request reviews at all.
Summarizing things which I said (or forgot to say) during meeting:
And I have a question: I have some unmerged stuff from ale, which I want to keep. (like orphans for logging inside ResourceT/MonadBaseControl IO, etc -- can I extract it somewhere) |
And regarding MPL license:
https://www.gnu.org/licenses/license-list.html#MPL-2.0 So MPL 2.0 is ok for me (problems, which are I talk about was possibly with 1.0) |
I should clarify that, I believe, everything I propose here does not contradict any of our company-wide policies: there is no company-wide workflow (there is one in CSL, but it is not formalised) and the styleguide is only augmented. If there are any contradictions with company-wide policies, they are bugs. The goal is to see how well these augmentations work and then try to push them to other projects. |
Regarding the username prefix in branch names, I had two issues with it:
The feedback I’ve got is that some people use it to quickly filter their own branches and there are some other use cases. Personally I think, the solution to the first concert is to use whatever naming you like in your local git repository. But in general, since there are people relying on this naming scheme, of course we’ll leave it the way it used to be. |
On Mon, Mar 05, 2018 at 06:33:11AM -0800, Kirill Elagin wrote:
* I don’t see how it is useful.
* I have a feeling that it encourages “branch ownership” and other team members might feel that their commits are not welcome to a branch, while I think that anyone should feel free to push these topic branches.
Well, if I see `kirelagin/YT-42-feature` branch, and would like to show
some possible improvement, which you $username like or dislike, is much
easier push `avnik/YT-42-feature` aside, where original dev, who start
topic branch free to cherry-pick and/or merge changes. Otherwise it can
bring us to conflict/rebase/merge hell.
|
For me, having several branches for the same feature feels like looking for a trouble: one should track what commits from what branches are already in and what commits are just waiting to be picked. It's much harder to mess things up when somebody pushes to your branch: you make changes, try to push them, server denies to accept your commit, you become aware that something have changed. This "conflict/rebase/merge hell" can occur only when two or more people are working on the same feature simultaneously. But conflicts are unavoidable in this case. Aside, this kind of conflicts are quite good, they mean that the work is being actively done on this feature right now! Yay, this feature will soon be done!
Well, this is a great git feature. But it's not used often (I used it in ALE only once or twice) and it's quite easy to forget something while using it: say, somebody pushed 3 commits but you cherry-picked only 2 of them and forget about the last one. This case, this third commit is lost forever. Also, |
On Mon, Mar 05, 2018 at 07:37:46AM -0800, Andrey Komarov wrote:
> is much easier push `avnik/YT-42-feature` aside, where original dev, who start topic branch free to cherry-pick and/or merge changes.
For me, having several branches for the same feature feels like looking for a trouble: one should track what commits from what branches are already in and what commits are just waiting to be picked.
It's much harder to mess things up when somebody pushes to your branch: you make changes, try to push them, server denies to accept your commit, you become aware that something have changed. `pull`, `merge`, `push`, no commits are forgotten.
This "conflict/rebase/merge hell" can occur only when two or more people are working on the same feature simultaneously. But conflicts are unavoidable in this case. Aside, this kind of conflicts are quite good, they mean that the work is being actively done on this feature right now! Yay, this feature will soon be done!
well, I want to be able to make `git push -f avnik/YT-42-bla-bla`
anytime, w/o care of conflict resolution.
May be we should use `feature/YT-42....` as shared branches, which
welcomed for pushing, and nickname prefixed as private (not-welcome
pushing, but allowed in some cases)
> cherry-pick
Well, this is a great git feature. But it's not used often (I used it in ALE only once or twice) and it's quite easy to forget something while using it: say, somebody pushed 3 commits but you cherry-picked only 2 of them and forget about the last one. This case, this third commit is lost forever.
I this case this private branch can be merged as is, cherry-pick is
exactly for case where you need commit 1 and 3, and sure that commit 2
is crap.
Also, `cherry-pick`ing loses commit signatures: if you cherry-pick a commit signed by me, this new commit will no more be signed by me. This doesn't happen when using `merge` (and also happens on `rebase`s). I'm not sure if this is bad, it's just a random thought.
`-x` (if I understand manual right) would copy signatures as part of commit
message.
Another good usage of cherry-pick, is extracting small features from
long-time feature branches (for example -- I have permanent WIP branch
on my nixpkgs)
PS and if I understand right, git skip cherry-picked from master commits
during rebase.
|
I like branch ownership 🤷♂️ |
0e9ecd9
to
914732b
Compare
914732b
to
41c77cc
Compare
Some notes:
|
FTR: I did a little googling and it seems (not surprisingly) that there are other people who arrived at this single-commit-per-PR idea and there are even projects that require this in their contribution guides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I like the idea with single commit. This also solves the issue with merge/rebase resolving strategies. It would be a good idea not to merge master into your branch but rebase. But that's a good point about making "Address review comments" in separate commit before somebody reviews it and squash everything when it ready to go to master. I'm voting for that but not for huge tasks because there are logically separated parts in such PRs that are easier to be read one by one most of the time.
On Tue, Mar 06, 2018 at 12:09:05AM +0000, Kirill Elagin wrote:
Some notes:
* I’ve got a crazy idea of enforcing a single-commit-per-PR policy (that’s what they do at Google, as far as I am aware) but I need to think about it.
I am against strict single-commit rule.
I like to make more than one logically separated commits (just because
they easy to cherry-pick to own PR, if developlemt of feature stalled,
or take too much time)
|
|
||
1. Starting from `master` create a new _issue branch_. | ||
2. Make your changes. | ||
3. Sign the most recent commit in your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I should sign all commits in my branch? I made PR recently, and it has disappointing red cross, which says that all commits must be verified by GPG signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to require it.
The integration we currently use does not have an option to only check the last commit, but there is an feature request for it, so the plan was to wait for it to get implemented and then make this check required.
Unfrotunately, now I see that the repository is archived, which probably means that the author no longer plans to develop it. We will have to either fork it or implement the check ourselves on our CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirelagin ok, now I understand that is redundant to sign all commits. The most recent commit signature protects previous tree from being changed.
So, when we are going to fix this? Would you create a ticket at YT?
- o-clock-0.1.0 | ||
|
||
# for serialise | ||
- cborg-0.2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cborg
comes after universum
? Does comment affect sort order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I know, commit squashing folds commit messages ( by default ). Does it mean that changes lose their commit messages? |
I think, we should include guide about signing commits with GPG signatures. E.g.: github argues at this PR cause some commits are not signed. |
@kirelagin regarding "Google uses single commit per PR policy".
To sum up, it's quite different from our situation, so please don't refer to their practices as the golden standard. |
@mitutee
There are plenty of guides including one from GitHub itself. What exactly do you want to put into our guide? |
I want to see clarification and rule, should we sign commits or not. ( I think we should, but I see that the half of the commits in, for instance, |
https://github.com/serokell/lootbox/pull/1/files#diff-2c773389d3a339fd1921f19e221ccdcaR37 |
copyright: 2018 Serokell | ||
license: MPL-2.0 | ||
github: serokell/lootbox | ||
extra-source-files: [README.md] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like extra-doc-files
is better name for list of README, CHANGELOG, etc.
@@ -0,0 +1,3 @@ | |||
* Use 2 spaces for indentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different style-guide recommendations for different languages might be ok if only this recomendations are language specific. I don't see any language specific in number of spaces for any reasonable language (both general purpose programming language and markup or configuration language).
@@ -0,0 +1,21 @@ | |||
name: lootbox-snapshot | |||
resolver: lts-10.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, latest resolver at the moment is lts-10.8
. Sometimes PR are not merged for a long period such that new resolver appears. I didn't notice policy regarding bumping up lts
. Something like bump up LTS resolver only if required (but not really rare, I don't want to have 20GB ~/.stack
folder because main projects uses LTS from year 2007) because we don't want to make cache invalid. Though I think that it's always a good idea to switch to latest GHC as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there were no LTSes in 2007.
resolver: lts-10.7 | ||
|
||
packages: | ||
- log-warper-1.8.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use log-warper-1.8.10.1
to not have problems with stack2nix
and have fix for some async exception handing.
commit: 858e76d8d92d683ff75f298ff39ca6b24daafaac | ||
|
||
# for log-warper | ||
- o-clock-0.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use o-clock-0.1.1
with log-warper-1.8.10.1
.
@@ -0,0 +1,8 @@ | |||
resolver: snapshot.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know about build time being decreased!
- code/network | ||
|
||
ghc-options: | ||
"$locals": -Wall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also want to include -fhide-source-paths
option:
Also, I didn't notice any word about how to maintaing CHANGELOG in repository. For libraries which are supposed to be uploaded to Hackage it should be clear. As well as versions policy: PVP or Semver or something different. You can see some examples in |
I think having |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding CODEOWNERS file.
* List dependencies from the same project in alphabetical order followed | ||
by an empty line. | ||
|
||
* List other dependencies in alphabetical order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, it's a bit ambiguous, because it's not 100% obvious how to order capital letters and lowercase letters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm just being blind, but I still fail to understand. What is the purpose of this repository, and how is it related to everything else we have?
docs/workflow.md
Outdated
@@ -43,16 +54,18 @@ Working on an issue | |||
Reviewing pull requests | |||
------------------------ | |||
|
|||
* Start from the pull requests where your review was explicitly requests | |||
* Start from the pull requests where your review was explicitly requested | |||
(you can use the `review-requested:<username>` search query). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also https://github.com/pulls/review-requested.
I think everything was addressed, but if not I’m open to suggestions
It is somewhat different from what we had in Ale:
code
) because the root directorywill inevitably get cluttered by various meta-files so this way
we at least won’t clutter it with our multitude of packages.
lib
instead of ambiguoussrc
.hpack
stuff lives inlootbox-base
.