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

Added error operator #1435

Merged
merged 3 commits into from
Dec 14, 2024
Merged

Conversation

kirjorjos
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Dec 14, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

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

Thanks for this @kirjorjos!

Could you also add some unit tests for this new operator in https://github.com/CyclopsMC/IntegratedDynamics/blob/d7b38c774b591aa357ff76d3d539a718fe6167ab/src/test/java/org/cyclops/integrateddynamics/core/evaluate/variable/TestStringOperators.java ?

The CLA check probably fails because your git user that created the commit is not linked to your github account. (you can see this, as there is no link behind your commit's username)

.inputType(ValueTypes.STRING).renderPattern(IConfigRenderPattern.SUFFIX_1_LONG)
.function(
(variables) -> {
throw new EvaluationException(Component.translatable(variables.getValue(0, ValueTypes.STRING).getRawValue()));
Copy link
Member

Choose a reason for hiding this comment

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

Is the translatable intentional here?
Could make sense to have that here indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw translatable getting used in other parts of the file to make a MutableComponent that the EvaluationException constructor expects, but if there's another way you'd prefer me make one, I can definitely change it.

@@ -1060,6 +1060,8 @@
"operator.integrateddynamics.string.name.info": "Get the string or name of a named object or converts it to a string",
"operator.integrateddynamics.string.unique_name": "Unique Name",
"operator.integrateddynamics.string.unique_name.info": "Get the unique name of an object",
"operator.integrateddynamics.string.string_error": "Error",
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to have gone wrong here. Could you use spaces instead of tabs?

@kirjorjos
Copy link
Contributor Author

I think I added the lang file indentation fix and tests for the operator, I'm not familiar with github enough to be 100% sure though if I'm being honest.

@rubensworks
Copy link
Member

rubensworks commented Dec 14, 2024

You can fix your git settings like this: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

GitHub Docs
You can set the email address that is used to author commits on GitHub and on your computer.

@kirjorjos kirjorjos force-pushed the master-1.19 branch 2 times, most recently from 733f51e to e96ad73 Compare December 14, 2024 13:42
@kirjorjos
Copy link
Contributor Author

Thank you for the documentation on fixing it, had to do some googling to retroactively change the email the commit used, but it's all working now and future ones will have the correct one.

@kirjorjos
Copy link
Contributor Author

oh, looking at the comparison, also fixed a typo in the concat tests and promptly forgot, if you need me to, I'll change it back, but I don't see anything where it should mater

@rubensworks rubensworks merged commit 4c4b861 into CyclopsMC:master-1.19 Dec 14, 2024
1 of 2 checks passed
@rubensworks
Copy link
Member

Super, thanks @kirjorjos!

@rubensworks
Copy link
Member

@kirjorjos I just noticed you submitted this PR to master-1.19, while it's actually master-1.19-lts that is being supported atm. Could you open a new PR for that one? master-1.19 is EOL.

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