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

Remove restrictions to rename a tool in ToolStation #136

Closed
wants to merge 3 commits into from

Conversation

Worive
Copy link

@Worive Worive commented Nov 15, 2024

@wlhlm
Copy link
Member

wlhlm commented Nov 15, 2024

Please run spotlessApply. CI is not happy.

@Worive
Copy link
Author

Worive commented Nov 15, 2024

Please run spotlessApply. CI is not happy.

Seems like I forgot to disable the IntelliJ IDEA "Optimize Imports", which has been performed just before commit 😅 Disabled for good now 👌

Copy link
Member

@chochem chochem left a comment

Choose a reason for hiding this comment

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

You should change it properly and not just switch that method to true while leaving dead code around it. That is if we actually want to change this.

I have to say I dont see why we would want to change this behaviour, it seems to work as intended. But I guess I don't really care.

@Worive
Copy link
Author

Worive commented Nov 15, 2024

You should change it properly and not just switch that method to true while leaving dead code around it. That is if we actually want to change this.

I have to say I dont see why we would want to change this behaviour, it seems to work as intended. But I guess I don't really care.

Alright, I'll commit that tomorrow 👍

@chochem
Copy link
Member

chochem commented Nov 15, 2024

can you also say why you want to change this?
the people in the ticket simply didnt know how it works, so that would not be a good reason.

@Worive
Copy link
Author

Worive commented Nov 15, 2024

can you also say why you want to change this? the people in the ticket simply didnt know how it works, so that would not be a good reason.

No reason, it was simply in "ready for development" so I checked it out. Did think it was already approved or something to have the label. I may have misunderstood the meaning of the label.

@chochem
Copy link
Member

chochem commented Nov 15, 2024

Not all tickets are correct ;)
there is a comment in the code explaining things, so I assumed you saw that. maybe you didnt.

@Worive
Copy link
Author

Worive commented Nov 16, 2024

Closing PR as the change is not needed any more. Thanks @chochem, for the clarification.

@Worive Worive closed this Nov 16, 2024
@Worive Worive deleted the fix-rename branch November 16, 2024 09:30
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.

Tinkers Tools cannot be renamed after initial craft
4 participants