search

alphazframework / framework

Core files of AlphaZ Framework
https://alphazframework.github.io/
MIT License
16 stars 17 forks source link

Fixes, improvements, tests (archive classes). #322

Closed Maikuolan closed 4 years ago

Maikuolan commented 4 years ago

Various fixes and improvements on the archive classes, and an attempt to write some tests for them (more details attached to commit descriptions).

I'm not quite as proficient at writing tests as I would like, so let's wait and see whether these tests pass before merging, in case anything isn't quite right. Fingers crossed.

Maikuolan commented 4 years ago

Hm.. Okay.. There are already some complaints from Sider and StyleCI. I'll try to fix those first.

Maikuolan commented 4 years ago

The method compress() has a Cyclomatic Complexity of 21. The configured cyclomatic complexity threshold is 10.

I'm not really sure what would be the best way to fix that last one. It seems, Sider doesn't like the number of if/switch/else statements..? I could reduce the number of such statements, but not without sacrificing readability, I think.

All the other Sider and StyleCI complaints have been fixed though.

Still waiting for the TravisCI results (pending.. pending.. pending.. hmm.. '^.^).

lablnet commented 4 years ago

The method compress() has a Cyclomatic Complexity of 21. The configured cyclomatic complexity threshold is 10.

I'm not really sure what would be the best way to fix that last one. It seems, Sider doesn't like the number of if/switch/else statements..? I could reduce the number of such statements, but not without sacrificing readability, I think.

I wondered to see in sider, i think we should remove sider from project, what do you think?

travis is too slow, in master branch we already moves to GitHub action

at end, thanks for your time to contribute.

peter279k commented 4 years ago

I think it should let zestframework:dev@220 compare to master branch to make current version consistency.

Maikuolan commented 4 years ago

I wondered to see in sider, i think we should remove sider from project, what do you think?

Could be worth considering. I don't use Sider at any other projects, so I don't have much experience with it, but I suspect that worrying too much about some of the complaints from Sider could potentially hinder further progress in the future, depending on the naturally expected complexity of possible future functionality that we may want to see implemented.

I think it should let zestframework:dev@220 compare to master branch to make current version consistency.

Good point. ^.^

Maikuolan commented 4 years ago

Done. :-)

I guess then, we can see whether the tests pass with GitHub Actions..? ^.^

lablnet commented 4 years ago

I guess then, we can see whether the tests pass with GitHub Actions..? ^.^

image

For now test pssing, and after the PR merge i think we can able to see either tests also working with github actions or not.

lablnet commented 4 years ago

This PR is ready to merge