Replies: 13 comments 19 replies
-
@tcheeric hi, eric. thx for the inclusion & your description reads well. pulling down the changes shortly to review the implementation and will get back to you soon. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. attempting
are still referenced throughout if i've perhaps overlooked something, pls advise- otherwise likely a 'git add/commit/push' on your end should resolve any discrepancies and ensure i'm looking at your intended branch state. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. supplemental to above: quick comparison of ~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ git diff --name-only upstream-develop |wc -l
395
~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ w/ diff file between the two branches nearly ~23K lines long: ~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ git diff upstream-develop | wc -l
23009
~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ containing ~7K lines of actual changes: ~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ git diff --unified=0 upstream-develop | grep -Po '(?<=^\+)(?!\+\+).*'|wc -l
7352
~/git/avlo-nostr-java-fork(0.7-SNAPSHOT_wip)$ so exceedingly likely your repo
|
Beta Was this translation helpful? Give feedback.
-
just did a quick re-read & saw your bullet point #8 (develop branch changes not yet merged into $ git merge-base 0.7-SNAPSHOT_wip upstream-develop
06407989fd09c39591b99dbdaf9d12577756fff8
$ git show 06407989fd09c39591b99dbdaf9d12577756fff8
commit 06407989fd09c39591b99dbdaf9d12577756fff8
Merge: e3023681 8ff8bb7e
Author: Eric T <tchepane_github@tutamail.com>
Date: Sat Feb 15 15:37:08 2025 +0000
Merge pull request #195 from avlo/req_message-validation
Req message validation in either case, when that merge is complete i'll be happy to take a look at the resulting implementation/diffs. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. am i assuming correctly that whereas current state of with a bit of work i've been able to get as i believe i've got a good understanding of what your refactor goals are, i would like propose i do a proper/surgical refactor (neither breaking any existing functionality, nor tests) to properly integrate your architectural/design changes for:
and request that 07-SNAP is not merged into develop branch prior to reviewing my alternate submission. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric supplemental to above: my variant will also include your new implementations for: NIP44.java
NIP44Impl.java
NIP60.java
NIP61.java
NIP61Impl.java
NIP04Event.java
NIP08Event.java
Amount.java
NutZap.java
NutZapInformation.java
SpendingHistory.java including your variants to: NIP<XYZ>.java classes, along with your new module / project-module restructuring: nostr-java-api/
nostr-java-base/
nostr-java-client/
nostr-java-crypto/
nostr-java-encryption/
nostr-java-entities/
nostr-java-examples/
nostr-java-id/
nostr-java-test/
nostr-java-util/ |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. do you have a working/uncommented variant of EventTagTest or working equivalent for any other class extending |
Beta Was this translation helpful? Give feedback.
-
Oki. Ping me anytime if there is anything I can help with
--
Secured with Tuta Mail:
https://tuta.com/free-email
6 Mar 2025, 19:35 by ***@***.***:
…
super, even better, thx & should have the remaining merge done within a day or so (assuming no other roadblocks/unforseens/etc)
—
Reply to this email directly, > view it on GitHub <#201 (reply in thread)>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQMG7BGRVDCZK6GANZWJK32TCPQXAVCNFSM6AAAAABYDPRYA2VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTENBRHAYTIMY>> .
You are receiving this because you were mentioned.> Message ID: > <tcheeric/nostr-java/repo-discussions/201/comments/12418143> @> github> .> com>
|
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. in-progress branch 07-SNAPSHOT_merge_fixes current merge-fix state, noteworthy:
notes, as per your interest:
after all tests are working again, there are a few other locations candidate for stream conversion as well. if you'd like to help, updating the remaining failing tests would be super helpful. lastly, below file-name legend for non-obviously-named temporary files:
|
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. additional merge fixes pushed to 07-SNAPSHOT_merge_fixes branch, additional unit tests now passing (116 pass, 13 fail = 129 total): looking into those now. once they all pass, removing all pre-07-SNAP event/tag/message implementations (a simple delete-en-masse), then renaming all RxR files to their proper/07-SNAP names, after which i'll push another branch for us both to evaluate prior to submitting a formal PR to the develop branch. suggestions, thoughts, ideas, concerns, etc are welcome as always. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. ran into an issue i'm unable to configure/resolve/understand, whereby an addressable tag having parameterized replaceable format:
specifically the:
portion is being (mis)interpreted as entirely kind: and thus causing test failure. perhaps/likely the above format requires a tags.properties configuration that is missing? if so, hoping you can properly configure, as it appears to require nesting- which i'm uncertain (mainly, oblivious) of how to do. i've pushed a branch to your repo w/ the single unit test added, located here. i'll work on this a bit tomorrow otherwise will take a break for the weekend as troubleshooting/debugging remaining test failures could definitely use a bit of stepping away / refresh for a bit. |
Beta Was this translation helpful? Give feedback.
-
@tcheeric hi, eric. after a couple runs attempting integration of below diagram and accompanying branch develop-compositional_architecture demonstrate an alternate architecture to code equivalent of above architecture (a compositional/decorator pattern) pattern preferable over inheritance in situations like ours), below: (note: middle ' if you can advise the proper git revision# of your work proceeding the "force-merge" with develop branch (should be prior to resultant
ideas/questions/concerns welcome as always and hope you're feeling better, dude. |
Beta Was this translation helpful? Give feedback.
-
hi nick,
I hope this helps. Also, wouldn't it be easier for you to add your changes to the current develop branch? Next, I am going to checkout your branch to reproduce the issue with the a-tag and will advise asap. Thanks |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Nick @avlo, this is a major refactoring, an attempt to rectify some of the bad initial design decisions, reduce the size of the codebase, and hopefully make it better. It's still work in progress, but the code compiles without error, and the test cases have been adjusted accordingly.
I would appreciate your opinion, suggestions, critique etc, when you have time, as it may also have an impact on SC.
Here are the major changes:
impl
classes in the API, they are not needed. SeeNIP01
re-implementation.GenericEventFactory
andGenericTagFactory
, to createGenericEvent
s andGenericTag
sBaseTag
(or rather, merged it with theGenericTag
)tags.properties
(base module) by providing its code, the list of its attribute names, and its corresponding nipfromGenericTag
method. This is useful. (seePubKeyTag
class example)fromGenericEvent
method is also helpful (seeTextNoteEvent
) but json serializer is not needed.develop
branchThanks.
Beta Was this translation helpful? Give feedback.
All reactions