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

Script can't handle class inheritance or class Instantiation #8

Open
reppiz opened this issue Apr 29, 2024 · 8 comments
Open

Script can't handle class inheritance or class Instantiation #8

reppiz opened this issue Apr 29, 2024 · 8 comments

Comments

@reppiz
Copy link

reppiz commented Apr 29, 2024

Would really love to have this fixed as its pretty important to capture class inheritance and instantiation when trying to parse Arma 3 classes.

When I try to parse a config.cpp file it errors out when it comes across the following.

class SlotInfo;

and

class my_class: other_class_inherited
{
       random data
};
@overfl0
Copy link
Owner

overfl0 commented Apr 30, 2024

Regarding class SlotInfo;, there is a Pull Request from @Adanteh here: #2 but since no one else seemed to really need this, I was pestering him with writing unit tests before merging this in. Sadly, it seems to now have conflicts with master :(

Regarding inheritance:
Do you mean that it breaks when trying to parse the inherited class? It doesn't to me:

>>> import armaclass
>>> armaclass.parse("""
... class my_class : other_class_inherited
... {
...        a = 5;
... };
... """)
{'my_class': {'a': 5}}

Or do you mean that other_class_inherited is nowhere to be seen?
If the latter, then this maybe should actually be stored somewhere. Not really sure where or how. We could either add meta-entries like my_class["__inherits"] = "other_class_inherited" but that could potentially mess with perfectly fine code.
Another solution that I see would be to create a class that inherits from dict so that we could then add an attribute to it, so that we would have my_class.inherits = "other_class_inherited".

Anyway, this would require changing the parser and while I'm actually currently working on this module, I was focusing on adding fuzzing before releasing the cython version of it, so I can't say when I will be able to add it myself. Maybe consider submitting a PR? ;)

@reppiz
Copy link
Author

reppiz commented May 1, 2024

If nothing else, being able to not error out when our classes don't always have beginning and ending curly brackets would be nice.

For the second point, I should have clarified. Yes, it does parse but it doesn't bring in the inherited class which I think is an oversight. Foy my specific case, If I lose the inherited class then it will break my config when I try to spit out the modified data back into an Arma class structure.

@overfl0
Copy link
Owner

overfl0 commented May 6, 2024

For the record: Commit 42a5d83 fixes empty classes. You can just use the master branch.

Regarding the other part, I've started working on it yesterday, but hit a few issues with Cython and those custom classes inheriting from dict(). I'll need to think about a general solution, because we also have delete, import statements and maybe some preprocessor stuff that would be good to keep, when regenerating the file, and I'd need to think about how to proceed with those.

1:1 parsing-generation matching was never really a goal (like ignoring comments) but these ones actually impact the functionality of the generated file, so it would be good to handle them correctly.
I'm also open to suggestions, if you have any.

@reppiz
Copy link
Author

reppiz commented May 8, 2024

Thanks for pointing that out. I didn't know master branch fixes that issue.

I unfortunately don't have any suggestions due to being unfamiliar with Cython. However, I appreciate your help and look forward to your solution.

@overfl0
Copy link
Owner

overfl0 commented May 8, 2024

The master branch fixes that issue because I've added it just before writing that last message so it's normal that you did not expect it to work on master (because it wasn't :) ).

Regarding suggestions, I was talking about the python part (as i'm writing everything in Pure Python mode with cython decorators, so that the code will work just fine without cython, it will just be much slower).
The suggestions were to be about whether to use dummy __inherits entries in the dict or create a class with klass.inherits attributes instead of the dict (in which case the attribute doesn't get printed while printing the dict, as it's not the class content, while still being able to be accessible through attributes).
That or maybe yet another way of dealing with inheritance. That information must be stored somewhere.

The downside of using a class Class(dict) that I just thought of right now is that those attributes won't then be serializable to json, which might be problematic :/

@reppiz
Copy link
Author

reppiz commented May 8, 2024

Would it not work to just change the key name in the dictionary from class to class.inherits when we come across inheritance in a Arma class structure? I think getting the attributes printed out is nice when printing the dictionary.

@dijksterhuis
Copy link

The downside of using a class Class(dict) that I just thought of right now is that those attributes won't then be serializable to json, which might be problematic :/

a dataclass might be helpful here? https://docs.python.org/3/library/dataclasses.html#dataclasses.asdict

@overfl0
Copy link
Owner

overfl0 commented May 24, 2024

a dataclass might be helpful here?

I'm afraid not. The classes' contents that have a dataclass decorator need to be known ahead of time. I think?
Maybe if there is a way to dynamically expand a dataclass while we're parsing the file could make me consider using it, but that doesn't solve the issue that we have here.

Namely, we need to have a new type of item(s) that would allow us to store additional constructs. In case of this issue, we're talking about the class' parent in the interitance hierarchy, but I'd like to do it in a way that would allow us to parse and store includes, deletes, imports in the same way.

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

No branches or pull requests

3 participants