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

Add dry run option #46

Closed
wants to merge 6 commits into from

Conversation

Minecraftschurli
Copy link
Contributor

@Minecraftschurli Minecraftschurli commented Jan 14, 2023

closes #45

@Kir-Antipov
Copy link
Owner

As I said in the related issue, I don't really like global flags that alter behavior of the whole program and turn it into something else. Especially when they are implemented the way you did it - this "dryRun" option seems like a corruption, which has spread throughout the codebase and spoiled every part of it. I currently work on a lot of updates for mc-publish, which includes cleaning up and refactoring my code. Sadly, this solution is quite opposite of what I'm trying to achieve. I can think of a way to implement such feature without ravaging the entire project (e.g. by proxying fetch and providing more verbose logs during these dry/debug runs), but I still need some justification for this to be done.

And about other things. b79decb has nothing to do with the original goal of this PR, while being a massive and breaking change on its own, so it definitely shouldn't be included here. I'm working on glueing semver and Fabric/Forge/etc ranges together, and this process has so much pitfalls and requires so many structural and design changes, that I definitely won't let something like that slide as a sidetracked commit in the PR that focuses on different objectives.

While I deeply appreciate your enthusiasm and desire to help, some things must be reverted and others should be reworked. A couple of tips for future PRs:

  1. Please, please, if you're planning to make a big PR with lots of changes, discuss it with the repo owner/members first, because your vision for a solution of some problem may be drastically different from the one of those who actually control the project, therefore it will inevitably lead to the PR being rejected, which will only make everyone sad, because it means that you spent your time and energy for nothing
  2. Stay true to the initial goal of your PR and don't include random features into it, even if they are cool and you think those should be in the project too
  3. If you see a project that distributes itself directly from its GitHub repo (like this one), do not include builds in your PR. It's up to maintainers to decide if they want to release a new version of their app/script/something else or not, therefore it's up to them to make a final build of the project before the said release. Redundant "build" commits only pollute the git history
  4. Do not include API keys in commits, even if you think that it's fine. It never is. For example, even if the key you provided does not grant access to modify or delete your personal mods (I sure hope it does not), CurseForge forbids public distribution of personal keys for the API, so they will be blocked in no time if lots of people begin to use them (like in this case). As a sidenote, "brand new" CurseForge API is useless for mc-publish, so I don't know why you did what you did
  5. Try to follow codestyle of the project you are making a PR for. E.g., as you can see I use import { foo } from "bar"; with spaces before and after curly brackets, while you changed some of the entries to import {foo} from "bar";, and so on

I see that you have put quite some work and thought in these changes, but they just don't fit the project the way they are now, so it's easier to start fresh with them. Sadly, with my heart bleeding I need to press that red button under my message and reject the PR. But I still greatly appreciate the try to make this action better!

@Minecraftschurli
Copy link
Contributor Author

  1. I forgot I had the PR on this branch and started adding features I needed
  2. (referring to 4) I asked on the CFDev discord how I should do this and they told me it's fine to include the API key for the new API in the code as it is a read only key

@Kir-Antipov
Copy link
Owner

I asked on the CFDev discord how I should do this and they told me it's fine to include the API key for the new API in the code as it is a read only key

That's why you shouldn't trust anybody on Discord :D
Here is a quote from CurseForge's Eternal Terms of Use:

Developer will moreover not demonstrate or provide Eternal* to third parties or allow such third parties to perform any of the aforesaid actions.

* - ... APIs, plugins and user interfaces (collectively, “Eternal”)

So, throwing a personal key into the public domain breaches an agreement between user and CurseForge.

Anyway, distribution of any API key is meaningless at best and absolutely disastrous at worst. Therefore, the rule of thumb here is - if you are about to commit one for everyone to see, you're doing something wrong, so you need to review your current approach.

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.

Add debugging features
2 participants