Skip to content

Add support for Stringified NBT objects - #92

Merged
jcxldn merged 15 commits into
masterfrom
feat/string-nbt-helpers
Feb 14, 2021
Merged

Add support for Stringified NBT objects#92
jcxldn merged 15 commits into
masterfrom
feat/string-nbt-helpers

Conversation

@aakatz3

Copy link
Copy Markdown
Collaborator

Cool new system to make building NBT strings easy and not awful

@aakatz3aakatz3 self-assigned this Feb 13, 2021
@aakatz3aakatz3 added the feature New feature or request label Feb 13, 2021
@aakatz3
aakatz3force-pushed the feat/string-nbt-helpers branch from 57a5e3b to 7591f25CompareFebruary 13, 2021 06:20
@codecov

codecovBot commented Feb 13, 2021

Copy link
Copy Markdown

Codecov Report

Merging #92 (c6b71ae) into master (b794f7b) will decrease coverage by 0.07%.
The diff coverage is 16.33%.

Impacted file tree graph

@@ Coverage Diff @@## master #92 +/- ##
============================================
- Coverage 20.39% 20.31% -0.08% - Complexity 111 133 +22 
============================================
Files 63 71 +8 Lines 2859 3012 +153 Branches 381 402 +21 ============================================
+ Hits 583 612 +29 - Misses 2261 2379 +118 - Partials 15 21 +6 
Impacted FilesCoverage ΔComplexity Δ
...bdogdiner/stickyapi/common/nbt/NbtCompoundTag.java0.00% <0.00%> (ø)0.00 <0.00> (?)
...bdogdiner/stickyapi/common/nbt/NbtJsonAdapter.java0.00% <0.00%> (ø)0.00 <0.00> (?)
.../dumbdogdiner/stickyapi/common/nbt/NbtJsonTag.java0.00% <0.00%> (ø)0.00 <0.00> (?)
.../dumbdogdiner/stickyapi/common/nbt/NbtListTag.java0.00% <0.00%> (ø)0.00 <0.00> (?)
...umbdogdiner/stickyapi/common/nbt/NbtStringTag.java0.00% <0.00%> (ø)0.00 <0.00> (?)
...dumbdogdiner/stickyapi/common/util/StringUtil.java96.72% <0.00%> (-1.62%)29.00 <0.00> (ø)
...mbdogdiner/stickyapi/common/nbt/NbtBooleanTag.java61.53% <61.53%> (ø)9.00 <9.00> (?)
...umbdogdiner/stickyapi/common/nbt/NbtNumberTag.java84.21% <84.21%> (ø)8.00 <8.00> (?)
...dogdiner/stickyapi/common/nbt/NbtPrimitiveTag.java100.00% <100.00%> (ø)1.00 <1.00> (?)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b794f7b...c6b71ae. Read the comment docs.

@aakatz3
aakatz3force-pushed the feat/string-nbt-helpers branch from 7591f25 to ad30a23CompareFebruary 13, 2021 06:26
kokumaji
kokumaji previously requested changes Feb 13, 2021

@kokumajikokumaji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR looks fine to me, except for the class names and some method names, would suggest reading up on Java's naming conventions (or we should agree on naming rules in the next meeting?).

Waiting on other admins to comment on this PR.

Comment threadsrc/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtBooleanTag.java Outdated
Comment threadsrc/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtCompoundTag.java Outdated
Comment threadsrc/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtCompoundTag.java Outdated
Comment threadsrc/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtStringTag.java Outdated
Comment threadsrc/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtTag.java Outdated
@aakatz3

Copy link
Copy Markdown
CollaboratorAuthor

I used toSNBT instead of toString to be explicit that it's converting to stringifies NBT/mojangson, rather than a JSON string or something else. I can change if people thing it would be better

@jcxldn

Copy link
Copy Markdown
Member

I used toSNBT instead of toString to be explicit that it's converting to stringifies NBT/mojangson, rather than a JSON string or something else. I can change if people thing it would be better

I wonder if something like toNBTString() would be a better name, should be the best of both worlds?

@aakatz3

Copy link
Copy Markdown
CollaboratorAuthor

Sure! we good for StringNBT then?

@jcxldn

Copy link
Copy Markdown
Member

NbtBooleanTag is correct for Google's Java Style Guide which we will be looking to implement, so should we go with that instead of NBTBooleanTag?

src: https://google.github.io/styleguide/javaguide.html#s5.2.2-class-names (UpperCamelCase class naming)

@kaylendog

Copy link
Copy Markdown
Collaborator

Merging on hold until after module refactor.

@jcxldn
jcxldnforce-pushed the feat/string-nbt-helpers branch from 1a29f3b to 72fc012CompareFebruary 13, 2021 23:32

@jcxldnjcxldn 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.

Reviewed all files.

@jcxldn
jcxldn dismissed kokumaji’s stale reviewFebruary 13, 2021 23:57

Review has been addressed.

@jcxldnjcxldn 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.

Looks good! (will look at github codeql next.done)

@jcxldn

jcxldn commented Feb 14, 2021

Copy link
Copy Markdown
Member

@jcxldn
jcxldn merged commit ef3442b into masterFeb 14, 2021
@jcxldn
jcxldn deleted the feat/string-nbt-helpers branch February 14, 2021 00:32
@jcxldnjcxldn mentioned this pull request Feb 14, 2021
3 tasks
jcxldn added a commit that referenced this pull request Mar 23, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

featureNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@aakatz3@jcxldn@kaylendog@kokumaji@ZachyFoxx