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 snapshot, logging and fix some errors #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ptgms
Copy link

@ptgms ptgms commented May 17, 2023

I've noticed some things that could be improved. I am listing the things I have changed:

  1. I have made the version flag optional, defaulting to latest version.
  2. I have added an "s" option for build flag, allowing users to get the latest snapshot build
  3. I have added a logging library with predefined prefixes, allowing for system journals picking it up as errors everytime.
  4. I have optimized some code (remove redunant breaks, for i, _ can be changed to for i)

If you want any changes, Ill make sure to add another commit to the PR.

Copy link
Owner

@jonas-be jonas-be left a comment

Choose a reason for hiding this comment

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

Some changes requested

versionFlag := flag.String("v", "", "Version or Version Group of the Project. You can use\"l\" to get the latest version.")
buildFlag := flag.String("b", "l", "(Optional) Build Number of the Project. You can use\"l\" to get the latest version.")
versionFlag := flag.String("v", "l", "(Optional) Version or Version Group of the Project. Defaults to latest version.")
buildFlag := flag.String("b", "l", "(Optional) Build Number of the Project. You can use\"l\" to get the latest version, or \"ls\" to get the latest snapshot. Defaults to latest version.")
Copy link
Owner

Choose a reason for hiding this comment

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

s instead of ls

Suggested change
buildFlag := flag.String("b", "l", "(Optional) Build Number of the Project. You can use\"l\" to get the latest version, or \"ls\" to get the latest snapshot. Defaults to latest version.")
buildFlag := flag.String("b", "l", "(Optional) Build Number of the Project. You can use\"l\" to get the latest version, or \"s\" to get the latest snapshot. Defaults to latest version.")

"papermcdl/pkg/latest"
"papermcdl/pkg/paper_api"
"strings"
)

func main() {
projectFlag := flag.String("p", "", "Project you want to download.")
versionFlag := flag.String("v", "", "Version or Version Group of the Project. You can use\"l\" to get the latest version.")
buildFlag := flag.String("b", "l", "(Optional) Build Number of the Project. You can use\"l\" to get the latest version.")
versionFlag := flag.String("v", "l", "(Optional) Version or Version Group of the Project. Defaults to latest version.")
Copy link
Owner

Choose a reason for hiding this comment

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

Snapshot feature would be nice in the version as well.

Suggested change
versionFlag := flag.String("v", "l", "(Optional) Version or Version Group of the Project. Defaults to latest version.")
versionFlag := flag.String("v", "l", "(Optional) Version or Version Group of the Project. You can use \"l\" to get the latest version or \"s\" for the latest snapshot. Defaults to latest version.")

infoFlag := flag.Bool("i", false, "(Optional) Show info or list available only.")
flag.Parse()

papermcAPI := paper_api.PapermcAPI{URL: "https://papermc.io"}

if *projectFlag == "" && *versionFlag == "" && *buildFlag == "l" && *infoFlag == false {
// gui.StartGUI(papermcAPI) if no flags are provided
Copy link
Owner

Choose a reason for hiding this comment

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

Remove unused line

Suggested change
// gui.StartGUI(papermcAPI) if no flags are provided

gui.StartGUI(papermcAPI)
return
}

projects, err := papermcAPI.GetProjects()
if err != nil {
fmt.Println("error getting projects: ", err)
util.LogCon("error getting projects: ", err, util.Error)
Copy link
Owner

Choose a reason for hiding this comment

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

Please use a loggin library not a login util for example https://github.com/sirupsen/logrus


import "fmt"

func LogCon(s string, a any, logType int) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use a loggin library not a login util for example https://github.com/sirupsen/logrus

@@ -52,26 +52,21 @@ func (p *PapermcSelector) EnterInput() error {
case "projects":
p.project = p.List.GetSelected()
p.ShowVersionGroups(p.project)
break
Copy link
Owner

Choose a reason for hiding this comment

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

Why remove the break Statements?

Copy link
Author

Choose a reason for hiding this comment

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

they are uneeded and redundant, the program functions the same. try it

Copy link

@MiraculixxT MiraculixxT May 17, 2023

Choose a reason for hiding this comment

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

EDIT: I mismatched Java and GO here, because both langs are looks pretty same and it is in relation with java services. Yes, in GO you are correct

Switches in java continue proceeding all cases until the end of the switch or a break statement.
For example the following code will output A B C (i run it):

switch ("a") {
            case "a":
                System.out.println("A");
            case "b":
                System.out.println("B");
            case "c":
                System.out.println("C");
        }

If the input would be a b only B C would appear in the output and so on. The break statement is important to stop running all further cases.

This is the reason why you are able to stack cases in java like the following:

switch (something) {
    case 1:
    case 2:
    case 3:
        System.out.println("Output");
}

Copy link
Owner

Choose a reason for hiding this comment

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

Okay you both kinda right. When you look at the go tour you'll see that in go they are not required but in other languages they are required: https://go.dev/tour/flowcontrol/9

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.

3 participants