Type hints

[schperplata]

Hey! Would you accept PRs with type hints? I find this package very useful and would be willing to invest some time in type-hinting this package in future months.

[jesper-raemaekers]

Thanks! Yes I'm willing (and grateful) to accept type hints. I had to check, but looks like type hints are supported from Python 3.5 and higher so no issues there.

[schperplata]

After investing about 3 hours to type hinting the code, it seems like we have some problems with architectural structure of the project. It is not uncommon for type hints to expose circular imports due to the tangled file/object dependency graph, and as it seems right now, circular imports are somehow making the code not operationable. Examples:

1. polarion.polarion has circular import with polarion.project
2. polarion.document has circular import with polarion.project and polarion.polarion
3. polarion.testrun has circular import with polarion.record
4. polarion.document has circular import with polarion.workitem
5. ...

What are your thoughts on this - did you experience problems elsewhere (tests, everyday usage, import order issues, ...)? I know most of these things are solvable by even more abstraction (and current implementation is far from poor), but these modifications are never quick and simple. They take time and might break some backward compatibility, as code sometimes must be moved to other places.

You can inspect the changes on this branch.

[jesper-raemaekers]

I've known there were circular dependencies, but i have not run into any issues with it apart from when i started this project some time ago. I cannot remember why this was not addressed then, but here we are.

As you examples show, this is mostly caused by a class creating a new class, but the new class 'needs' a reference to the creator. Polarion creates Project, but Project needs polarion. Then Project createsd Workitem and workitem needs Polarion and Project.

So I'm happy to improve the structure, but I would need some suggestions on what and how to change it. Maybe if you have an example, I can 'simply' apply it everywhere.

[schperplata]

Maybe a good starting point is to draw a layout of Polarion itself, and see what are the relations between its parts. For example, after a connection is created, our entry point is a Polarion Project. It can hold many items, for example WorkItems, Documents, TestRuns, ... Furthermore, Documents (and others) can also hold WorkItems.
That directly means that Project and Documents are users of WorkItem(s), not vice versa. WorkItem has only dependency on Connection, but not on any of these other items.

... and so on. To set a more direct example, maybe something like this is the way to go:

1. polarion/polarion.py is a "root" file - it does not reference any other file/object. It has no dependencies other than to files in base directory. Polarion class can stay as it is except getProject() method, which could be moved to polarion/project.py in form of a public function or Project class static method. This function receive Polarion class instance as an attribute. Other files can than always import polarion.py as it is free of any dependencies. In example above, this is a Connection.
2. project.py on the other hand needs majority of other objects (WorkItem, Document, ...). That explicitly determine the relations: project.py imports other files, and not vice versa. Other files can import only other low level files, like workitem.py and polarion.py.
3. workitem.py, as stated above, is an "isolated object" from project.py - it does not import it or anyhow interact with its instance directly. Objects of other type are never created from WorkItem itself.
   The same goes for some other items, record.py for example. 4.WorkItem creator (__init__()) logic to find and build WorkItem from Project should be moved to Project itself, for example, into the getWorkitem() method.
   Maybe in workitem.py there should be more helper functions/methods to create WorkItem (new or from existing data) instead of current "large" constructor?
4. Back-links or "creator references", could be solved by items holding only IDs of the referencing object, not the object instance itself. User can than decide what to do with this ID, and if necessary find parent item via that ID and appropriate helper function/method. I am aware this is not as convenient as it is now, but that way you keep things separated and you get the option to completely refactor entire parent referencing object, without actually modifying child object at all - you only keep an ID, not the object itself.
   If performance is the issue (user finding parent item and re-creating objects), you could implement object caching (found by object ID) inside polarion package or similar.
5. I can see that in workitem.py there are helper functions to get WorkItem statuses (enum), which use project instance. As this is probably the only way (designed by Polarion), getters for enums maybe shouldn't be there but in Project itself, as they are not limited to one specific WorkItem, but work items of that specific type.
   btw: setStatus() does not warn user if new status is not known (which is dangerous, like caught exception with only pass in except block).

Now, this is just an example based on very limited knowledge of your actual implementation/package/caveats. I might understand it wrong or can't see all use cases, and even if I could, there might be many solutions to this problem and even when choosing one, you can't be sure it is the right one - such knowledge comes from experience and is hard to write down a "how-to guide".
Main point here is that relations between your objects are always in one-way, otherwise you will keep getting circular imports which are a good indicator, that object dependency graph is tangled IMO. If you search google images for code dependency graphs, you will see, that relations are always one way from object A to object B.

Hope this is of any help. This would be a massive change, which you might not be willing to do, and I can understand it.

[jesper-raemaekers]

Thank you for additional wording. i will get back to this at some point, but it should not be expected anytime soon.

[jesper-raemaekers]

Hi @schperplata ,

It has been a while and i have something that I'd like to share and maybe get your thoughts on. Is start by (as suggested) removed the connection element out of the class structure as is should hold no function there apart from allowing a connection.

So this class only knows the API and provides a wrapper for it. I've decided to add an abstraction layer here since there will be a REST API coming to Polarion and this way it is prepared. But it is currently unknown whether it will have the same function, complementary functions or a complete replacement of WSDL.

After this a set of base classes are defined with (potential) common functionality. These are used by various Polarion items, basically as used currently, but a bit more separated.

And finally all Polarion items. The smallest being a user, which is merely a info container. Workitem being the smallest Polarion item and thus can be referenced by all and may not reference anything but itself (needed for linking). If there is something where it would need to point to a bigger object, that must then be done via the project class.

The Plan, test run, record and document may all use workitem and user (left our of drawing for clarity). There should be no need for those object to reference each other. Finally Project creates all. All object to know about the project, but only because it is contained in their Polarion data. This will not be an object reference.

I hope you may have some feedback on this.

After this I plan to do the following:

• Find out how to mark stuff as deprecated so users can get a early warning
• Create this structure in a new branch and see if i missed anything
• Start creating this implementation

While doing this I also want to change the coding style to be more compatible with PEP8 as the names currently have always bothered me. Also this will mark the time it will be switched to a version number using year and moth. so .. as this is more compatible with Polarion version numbering.

While addressed @schperplata I encourage everyone who reads this to reply with an opinion.

[schperplata]

Hey! Yeah, that is exactly the way I see the layout of this project. Your thoughts match mine (above) and if properly implemented, will surelly address issues that type hinting exposed.
I can't be 100% sure about all the object relations, as I don't know all the details and Polarion-side gotchas. Anyway, the goal is to create an isolated objects that would be easy to develop, test and maintain. Also (same as you proposed), enoguh abstraction must be implemented on all layers so that adding another connection/feature can be done without changing current WSDL part or other un-related functionality. I think this is the way to go, we can look at the details further down the road, if necessary.

What I am a bit more concerned is the last part:

While doing this I also want to change the coding style to be more compatible with PEP8 as the names currently have always bothered me. Also this will mark the time it will be switched to a version number using year and moth. so .. as this is more compatible with Polarion version numbering.

In my point of view, all proposed changes (which are more than welcome in my opinion) represent major changes: either breaking backward compatibility or have a lot (A LOT) of extra code and temporary code mess. What I would do in this case is either:

1. Increase major package version and left current implementation intact.
2. Create completely new package.

I know these are hard to make decisions and no decision is pleasant as it affect users or maintainer. I personally would go into point 1.

In any case, I'm glad that you found time and a willingness to improve this package. I am currently quite busy, but if you need help, there is always a minute or two to spare (as long as there is no ETAs :) )

[jesper-raemaekers]

Thanks for your feedback. I would agree with you on versioning approach 1. So make the current state version 1.x.x and keep that there. There can be bugfixes on it, but it will be dropped eventually. The new version will start with 2022.x.x and will be break on nearly all fronts. There will be a clear note of this on the readme page.

I would appreciate your time for some feedback when needed :)

[jesper-raemaekers]

Hi @schperplata I'm back with a question.

I'm running into some trouble at abstracting the API's. The WSDL API return some objects that represent all polarion items. However the REST API returns everything nicely in JSON structures.

I'm thinking of going through the effort of converting everything from WSDL to JSON as well. This moves some complexity from other parts to the WSDL API but with this comes the bigger potential for bugs as now we have to touch all fields of a workitem for example. Converting the REST API output into objects from WSDL is probably not an option because we cannot presume the WSDL objects they will always be available.

Alternatively the abstraction point could be the actions that we preform on an object. So 'update field' for example. But this moves the functionality from what would be the Workitem class to the API and that seems to make little sense to me.

I would like to hear your thoughts on this.

[schperplata]

I think this might be a good example of a various design patterns (depends which ones on a chosen concept), for example adapter pattern.

My instinct also tells me what you proposed, that is an abstraction layer above I/O data that can either come from JSON, WSDL, or who knows, maybe some SQL database one day. As (I assume) Polarion items (objects) aren’t very varying part, I would probably create a Polarion-like models of items completely separated from any I/O data. Actual objects creation could benefit from another pattern, that is factory pattern, while I/O data manipulation could use adapter pattern to transform data JSON/WSDL.

Theoretically 😉 Practically implementation often depends on a bunch of other factors and compromises. Also, I am by no means some badass expert, so there might be a lot of other better solutions, opinions and experiences. I am also still learning design patterns and will continue to do so for the next few years. Light years 😂 However, I think we might be on the right path with abstraction of actual Polarion objects (models) and low level data manipulation.

Hopefully the response is of any use. If design patterns are something you aren’t familiar with (I’m sure you are), check for the book ‘Head first design patterns’.

[schperplata]

@Zoli2000 I am not an owner of this repo, but still: This is not the right place for your problem - please open a new issue, as this has nothing to do with a debate here. It is a common practice across GitHub (and other) projects. Thanks.

[github-actions[bot]]

This issue is stale because it has been open for 30 days with no activity.

[github-actions[bot]]

This issue was closed because it has been inactive for 14 days since being marked as stale.