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

id_gen.go too large #679

Open
sruehl opened this issue Aug 23, 2023 · 10 comments · May be fixed by #680
Open

id_gen.go too large #679

sruehl opened this issue Aug 23, 2023 · 10 comments · May be fixed by #680

Comments

@sruehl
Copy link
Contributor

sruehl commented Aug 23, 2023

at the moment id_gen.go is a pretty big file. Github has trouble rendering it and my IDE (Goland) just bails because the file is too big (it even can't resolve symbols so you get red errors everywhere).

image
image

Looking into it I had the idea to split the file up into 1+n files where n is the number of NodeClasses (ref). So we would have on common.go for the Name function feeding from n maps and a id_${NodeClass}_gen.go file for nodes grouped by class.

so codewise nothing would change as the name map is private anyway at the moment, we would just split up the load between multiple files.

@sruehl
Copy link
Contributor Author

sruehl commented Aug 23, 2023

I named common.go to id_names_gen.go in the PR

sruehl referenced this issue in apache/plc4x Aug 28, 2023
@sruehl
Copy link
Contributor Author

sruehl commented Aug 29, 2023

Edit: move PR comment on PR #680

@kung-foo
Copy link
Member

I'm kinda on the fence about this. On one hand, I don't like adding complexity and code paths specifically to address IDE and web interface deficiencies, on the other hand, I also use Goland and have hit this issue before (though I "solved" it by bumping up Goland's limits). @magiconair thoughts?

@sruehl
Copy link
Contributor Author

sruehl commented Aug 31, 2023

I can follow the argument and usually have the same stance that it doesn't make any sense to alter code because some IDE is unhappy. But even using VI or any editor this file is just too big 😄.
One note: I utilize the categories (type or whatever edit: NodeClass) the OPC-UA spec provides in the last column of the CSV. So from the spec those IDs are split up already. Splitting them up even more like I suggested would then fall again in the argument I brought up in the first sentence. So practically besides issues in Goland I had trouble generating code from it in plc4x using Java as it would either overflow the maximum number of enums supported 65535 or it would just overflow the static initalizer with too much bytes.
But as I wrote above: It doesn't hurt to segment. The imports/api look completely identical it is just split up into multiple files and makes it even more readable using any editor/viewer.

(edit: referring to #680 in this comment)

@magiconair
Copy link
Member

magiconair commented Oct 23, 2023

Hmm, this works just fine in VSCode and neovim. Isn't this a limit that you can tune in Goland? Seems like an arbitrary limit.

I could live with splitting the enums and the map into two different files but making a random split because of a Goland limit seems pretty weird. Let's try to fix the IDE first.

image

image

@magiconair
Copy link
Member

I also don't follow the code-generation limit argument. The number of enum values does not change. So why would splitting into multiple files not hit that limit?

Also, since the code generator is java you should be able to just provide more memory with -Xmx or not?

I don't think we had such a situation before and I'm also a bit on the fence here. On the one hand this code should just work. On the other hand I don't know how many people actually have this problem and whether the ones who do haven't just updated their settings and moved on. That's what I would do at least.

We can add something to the README if there is a simple workaround. If there isn't and the library is otherwise unusable with either tool then we should fix it.

@magiconair
Copy link
Member

magiconair commented Oct 23, 2023

If we have to split it up then I'd suggest we do this by id since this doesn't require any complex semantics or regular expressions. For example

id_00000_gen.go
id_10000_gen.go
id_20000_gen.go
...
id_names_gen.go

@magiconair
Copy link
Member

@sruehl is this still relevant?

@magiconair
Copy link
Member

I've just asked a friend to check this and he says that there is a button to enable the syntax highlighting with the suggestions to bump the memory and the instructions to do so.

@sruehl
Copy link
Contributor Author

sruehl commented Nov 30, 2023

yeah, but as I said this is still a pretty big file and splitting it up with the categories they supply upstream (#680) helps readability.
So even if I fix it for my particular IDE, Github still doesn't render it well.

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 a pull request may close this issue.

3 participants