Clean up architecture - Githubissues

smash-transport / sparkx

SPARKX - Software Package for Analyzing Relativistic Kinematics in Collision eXperiments

https://smash-transport.github.io/sparkx/

GNU General Public License v3.0

5 stars 0 forks source link

Clean up architecture #268

Closed NGoetz closed 2 weeks ago

NGoetz commented 2 months ago

This is a draft PR, because there are multiple ToDos left:

[x] Check if documentation still compiles
[x] Decide if Oscar and Jetscape should keep their name and position, or should move to the storers
[x] Add static typing
[x] Rebase
[x] Decide if analysis classes should act on BaseStorer in future, so that we don't have to get the particle_objects_list anymore
[x] Apparently we want to test ABCs now too, so there we go, we need ABC test classes
[x] Apply the formatter
[x] Check if I can document inherited methods

I want you two to go through this till the next sprint nevertheless. I need you to tell me if the architecture makes sense for you like this, if I broke anything/forgot anything, if any improvements are wanted etc. etc.. Be strict.

NGoetz commented 2 months ago

Btw wherever it says decide, your comments are appreciated.

NGoetz commented 2 months ago

Dummy and BaseStorer are moved to the directory. Changing to object analysis will be done in a separate PR.

NGoetz commented 2 months ago

Static typing has been taken care of.

NGoetz commented 2 months ago

There is one weird test which is failing, and the documentation has to be written. Apart from this, the PR is ready to go. Regarding the formatter: locally if I run the same command as in the github action, it tells me that nothing is to be changed. There must be some issue with it. I have version 21.12, pytoml is identical. Any idea @Hendrik1704 ?

Hendrik1704 commented 2 months ago

Can you try to run with a similar setup as in the workflow:

- name: Install dependencies
      run: |
        python -m pip install --upgrade pip
        pip install black==24.8.0

    - name: Check code formatting
      run: |
        black --check --line-length 80 src/sparkx tests/

NGoetz commented 2 months ago

Yes, it says again 49 files left unchanged.

Hendrik1704 commented 2 months ago

I don't understand it.

NGoetz commented 2 months ago

Can you check it out and test it locally?

On Wed, 28 Aug 2024, 19:27 Hendrik Roch, @.***> wrote:

I don't understand it.

— Reply to this email directly, view it on GitHub https://github.com/smash-transport/sparkx/pull/268#issuecomment-2315900836, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADTRQNYGFWHP4RQRGIKFWJDZTYCBTAVCNFSM6AAAAABMEV2J4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJVHEYDAOBTGY . You are receiving this because you authored the thread.Message ID: @.***>

Hendrik1704 commented 2 months ago

I applied the code formatter locally and now the action is successful. Only the issue with one of the tests is still there.

NGoetz commented 2 months ago

Though that fixes the formatting action, I still wonder why the formatter on my machine was not working in the first place. We should figure this out. Especially because it was the same version of black after all. With the current state it would tell me to reformat.

Hendrik1704 commented 2 months ago

I've changed the formatting action such that before the format check it performs a reformatting of the branch and if there is something not correctly formatted, it will create a commit (see last two commits).