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

Fix cyclic ref #141

Merged
merged 7 commits into from
Jan 16, 2023
Merged

Fix cyclic ref #141

merged 7 commits into from
Jan 16, 2023

Conversation

sbcgua
Copy link
Owner

@sbcgua sbcgua commented Dec 28, 2022

No description provided.

@sbcgua
Copy link
Owner Author

sbcgua commented Dec 28, 2022

@larshp The exception does not refer to ZIF_AJSON anymore but the error is still there ... looks like some bug.
image

@larshp
Copy link
Collaborator

larshp commented Dec 29, 2022

hmm, yea, its broken

@larshp
Copy link
Collaborator

larshp commented Dec 29, 2022

just disable the rule for now, the interfaces are cyclic anyhow, but will probably work in the merged report

@sbcgua
Copy link
Owner Author

sbcgua commented Dec 30, 2022

@larshp Ok, I picked the fix to the master, but left this PR open. Currently it contains just the setting to enable cyclic check, so it can be restarted and merged later when the issue is fixed

@larshp
Copy link
Collaborator

larshp commented Dec 31, 2022

reported cycle fixed with abaplint/abaplint#2822

@sbcgua
Copy link
Owner Author

sbcgua commented Jan 8, 2023

Now it is filter and mapping interfaces, they refer to types defined in zif_ajson (ty_node in particular). Hmm. Honestly speaking, semantically, the types are in a good place. Yet technically they create a cyclic ref. Maybe it makes sense to separate ty_node and Ko to zif_ajson_types or similar.

Any serious impact on AG or other tools you maintain ? @larshp @mbtools @albertmink @jrodriguez-rc

@larshp
Copy link
Collaborator

larshp commented Jan 8, 2023

if its all good, design wise, just exclude one of the interfaces from the rule

@mbtools
Copy link
Contributor

mbtools commented Jan 8, 2023

Minimal impact on my projects.

@jrodriguez-rc
Copy link
Contributor

No impact from my side

@albertmink
Copy link
Contributor

No, it is fine.

@sbcgua
Copy link
Owner Author

sbcgua commented Jan 16, 2023

Ok, so most of types (except ty_opts) were move to zif_ajson_types. cyclic_ref rule re-enabled.

Let's hope nothing breaks ... merging

@sbcgua sbcgua merged commit b40ccfd into master Jan 16, 2023
@sbcgua sbcgua deleted the ag-fix-cyclic branch January 16, 2023 20:04
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.

5 participants