Skip to content

Move ReleasesApiConsumer to a separate file and do simple refactors#3188

Merged
robertbrignull merged 11 commits into
mainfrom
robertbrignull/releases-refactor
Jan 3, 2024
Merged

Move ReleasesApiConsumer to a separate file and do simple refactors#3188
robertbrignull merged 11 commits into
mainfrom
robertbrignull/releases-refactor

Conversation

@robertbrignull

Copy link
Copy Markdown
Contributor

This PR aims to do some simple refactors of the ReleasesApiConsumer class and associated code. There is more refactoring I want to do but I'm breaking it up into multiple PRs.

Each change is made in a separate commit, and a lot of it is just moving code around, so I recommend looking at each commit individually.

The changes we make are:

  • Move various classes out of distribution.ts and into their own files
  • Use newer methods of defining fields instead of having to prefix with underscores
  • Add / adjust comments on classes and methods
  • Move the tests of ReleasesApiConsumer out of the no-workspace tests and into unit-tests so they'll be faster
  • Merge owner name and repo name into a single repo NWO field, because after Remove ownerName and repositoryName from DistributionConfig #3183 we no longer need to be able to specify them individually anywhere

This PR deliberately does not attempt to change the parameters of the getLatestRelease method. I think that change will be a little more complicated so I wanted to save that for another PR.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team January 3, 2024 12:39
@robertbrignull robertbrignull requested a review from a team as a code owner January 3, 2024 12:39

@koesie10 koesie10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. However, perhaps all files related to the distribution could be moved into a codeql-cli/distribution directory? It seems like files like github-api-error.ts don't belong together with the CLI code directly. If you're already doing this in a follow-up PR, that's fine as well.

@robertbrignull

Copy link
Copy Markdown
Contributor Author

However, perhaps all files related to the distribution could be moved into a codeql-cli/distribution directory?

That's a good idea. I'll move them to that directory in this PR to keep all file file-moving in one PR.

@robertbrignull

Copy link
Copy Markdown
Contributor Author

I've moved the files into codeql-cli/distribution but I haven't moved distribution.ts yet as it would create slightly ugly imports like distribution/distribution. We could solve this by a index.ts file, or just continue the work of decomposing distribution.ts into smaller and better-named files. We can also just move it into the distribution directory and that's fine.

Since it's so easy to move files around I'm not too bothered about getting it perfect right away, as we can always tweak things. So I won't hold up this PR any more and will merge it now.

@robertbrignull robertbrignull merged commit 92bebef into main Jan 3, 2024
@robertbrignull robertbrignull deleted the robertbrignull/releases-refactor branch January 3, 2024 16:24
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.

2 participants