Closed spikespaz closed 1 year ago
Hi, @spikespaz! Thank you for a PR. I have a few comments:
alejandra
due to https://github.com/kamadorueda/alejandra/issues/372. That's why, I set nixpkgs-fmt
as a default formatter. Just run nix fmt
from the root.flake-utils
.git reset --soft
ed your commits because I wanted to simplify working with your PR and change it a bit. But I mentioned your input in the commit message https://github.com/nix-community/nix-vscode-extensions/commit/3198e7e0da697d023d665123e585527c37cc06ad!Overall, your important changes are now in master
\1. I use Alejandra because it is strict, at least that's what I got from discussion with others. I like a strict formatter because it helps with diffs. Does nixpkgs-fmt
really do better? I thought Alejandra was less buggy than the other two, but anecdotally it is still buggy. I have a few issues with the formatting of string-interpolated expressions.
\3.a I'm not really sure how commits are supposed to be for contributions. What didn't you like about the way I did it? I strive for information-density (that is, more descriptive, most brevity) and try not to change more than one "measurable aspect" of code at a time. I'd especially like some feedback on this because I intend to contribute more of my work back into the ecosystem.
\3.b What is the "correct" way to merge in contributions? As I understood, the preferred method is to cherry-pick squash, make and amendment commits. Did you elect to soft reset and adapt the code because it is easiest, or because it follows a particular methodology?
I would like to add to this PR, but I'm not sure how to do it. Should I hard-reset my branch to match current main
and continue working, or squash my commits and adapt them to match your changes?
Some things I would like to do:
vscode-marketplace
and open-vsx
) into outputs.packages
. Because outputs.extensions
is non-standard, and is only named in such a way that is specific to the purpose of this flake, I would also like to either deprecate or remove it. So far there hasn't been much concern about backwards-compatibility, considering this I would personally prefer removal.nixpkgs-fmt
. This ideally should be done in a separate commit so that only functional differences will be recorded by future commit diffs.old
?Addressing my own comment:
I have removed some metadata from the packages generated from Open VSX Registry.
meta = with lib; { inherit changelog downloadPage homepage; license = licenses.${license}; };
Is the missing metadata important? I have yet to find anything to generate static websites from such attributes (I would like to know how to do so, for example with module options) so I question the importance. Reintroducing this would further complicate code-path, but I don't want to be redacting anything significant (especially license metadata).
I had a hard time figuring out where changelog
, downloadPage
and homepage
come from. It is confusing because several namespaces are merged with with
, especially the extension attributes from the generated fetchers. It is clear to me that licenses
comes from vscode-utils
in nixpkgs, but that is all I could discern. I was inclined to think these attributes that were inherited don't even exist anymore; that they were removed by a change to the generated fetchers.
Later down the road:
Turn all machinery into a monolith. I see scripts
and hs
, and from guessing, the Haskell code relies on the scripts. Having all of these components written in different languages, and CI workflows in different places, is confusing and difficult for me to follow and start working with. I would like to eliminate this problem so that future maintenance would be easier.
How expensive is the GitHub CI that keeps this thing updated? I would like to reduce execution time rather than waste whatever public funding keeps the @nix-community org up-and-running.
Or, if you think all of this is a waste of effort, please do tell.
3.a + 3.b. Our flake is small, and your changes logically were not that dramatic and concerned a single file. That's why, I decided to choose the simplest way to work with them. Later, I discovered a way to list co-authors in a commit message and included you (see https://github.com/nix-community/nix-vscode-extensions/commit/46be1e77328c79878e698dd075842c7552546007). To add, I asked to enable Contributors in this repo.
If you'd like to send a link with Git best practices, I'd be eager to study it. For now, if you focus on a single feature, make commits for it, then git rebase -i
them to squash and combine commit messages. Rebase on master while working on a PR,
Some things I would like to do:
- Our template depends on our root flake. I think there shouldn't be a reverse dependency.
- The purpose of creating extensions is to let our flake be indexable by
nixos-search
(see https://github.com/nix-community/nix-vscode-extensions/issues/2). Without this smart move,nix flake check
won't work.- No need to format the generated Nix files, I guess. However, we may include the template file (see the
formatter
attribute inflake.nix
)- That's a good question! I'll describe it here (https://github.com/nix-community/nix-vscode-extensions/issues/10).
Addressing my own comment
@AmeerTaweel explained (https://github.com/nix-community/nix-vscode-extensions/issues/1) why he didn't fetch the meta info from vscode-marketplace
. When replacing nvfetcher
with a Haskell script, I decided to omit the additional info about packages from open-vsx
also to reduce the file sizes and to unify the fetcher's functions. We may add these attributes in the future after a discussion (https://github.com/nix-community/nix-vscode-extensions/issues/1).
Later down the road
The action is cheap. It takes 5 mins (see https://github.com/nix-community/nix-vscode-extensions/actions) . I created an issue about rewriting the scripts (https://github.com/nix-community/nix-vscode-extensions/issues/10)
Thank you for the feedback.
Regarding Git best-practices, I've yet to find a solitary source that details collaborative Git behaviors. The only way I've learned is through criticism from others.
The way that the overlay was set up was fundamentally flawed. When discussing issues I was having on the unofficial Discord server, I learned how overlays are supposed to work, and have now fixed the default overlay for this flake.
I have also taken the liberty of re-writing the entire thing. In order to make the entire process of turning the generated fetcher expressions into derivations simpler, I have removed some metadata from the packages generated from Open VSX Registry.
If you are okay with the invasive nature of the changes I've made, I'd like to keep going and replace more of the code here.