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

Rewrite flake (fix overlay) #9

Closed
wants to merge 6 commits into from

Conversation

spikespaz
Copy link
Contributor

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.

@deemp
Copy link
Collaborator

deemp commented Feb 3, 2023

Hi, @spikespaz! Thank you for a PR. I have a few comments:

  • I dislike alejandra due to Keep comments attached to the below line while in inherit kamadorueda/alejandra#372. That's why, I set nixpkgs-fmt as a default formatter. Just run nix fmt from the root.
  • I guess it's better to let a person be able to override the systems used in our flake, so it's better to keep flake-utils.
  • I git reset --softed 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 3198e7e!

Overall, your important changes are now in master

@spikespaz
Copy link
Contributor Author

spikespaz commented Feb 3, 2023

\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:

  1. Remove duplicate code. The example package is defined both in the flake, and the template. I think I would like to move my edited example package into the template, and import it from the flake.
  2. Introduce the extension registry namespaces (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.
  3. Format the rest of the Nix code with nixpkgs-fmt. This ideally should be done in a separate commit so that only functional differences will be recorded by future commit diffs.
  4. Document how the fetcher works, but I need your help with that. I doubt anyone cares much about it, but I'd like to understand how the machinery works. For example, one question I have is why do we record extension data in both Nix and YAML formats, what is the YAML used for, and why is it named 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.

@spikespaz
Copy link
Contributor Author

#8 answers my question regarding the missing metadata.

@deemp
Copy link
Collaborator

deemp commented Feb 4, 2023

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 46be1e7). 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:

  1. Our template depends on our root flake. I think there shouldn't be a reverse dependency.
  2. The purpose of creating extensions is to let our flake be indexable by nixos-search (see Add nix-vscode-extensions to nixos-search #2). Without this smart move, nix flake check won't work.
  3. No need to format the generated Nix files, I guess. However, we may include the template file (see the formatter attribute in flake.nix)
  4. That's a good question! I'll describe it here (Make a monolith fetcher #10).

Addressing my own comment

@AmeerTaweel explained (#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 (#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 (#10)

@spikespaz
Copy link
Contributor Author

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.

@spikespaz spikespaz closed this Feb 5, 2023
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.

None yet

2 participants