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

v0.32.0 #367

Merged
merged 39 commits into from
May 3, 2022
Merged

v0.32.0 #367

merged 39 commits into from
May 3, 2022

Conversation

stolarczyk
Copy link
Member

@stolarczyk stolarczyk commented Jun 2, 2021

todo

  • update version
  • add tests for new features
  • wait for v0.13.1 attmap#76
  • update attmap requirement
  • pass tests
  • update changelog
  • test manually in some real projects
  • Make sure it's tested with looper before release

@stolarczyk stolarczyk marked this pull request as draft June 2, 2021 17:27
@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #367 (b94aaa9) into master (3f02ec6) will increase coverage by 22.92%.
The diff coverage is n/a.

❗ Current head b94aaa9 differs from pull request most recent head c76c574. Consider uploading reports for the commit c76c574 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #367       +/-   ##
===========================================
+ Coverage   65.19%   88.12%   +22.92%     
===========================================
  Files          14       10        -4     
  Lines        1428     1187      -241     
===========================================
+ Hits          931     1046      +115     
+ Misses        497      141      -356     
Impacted Files Coverage Δ
peppy/_version.py 100.00% <ø> (ø)
peppy/const.py 100.00% <ø> (ø)
peppy/exceptions.py 74.07% <ø> (+2.07%) ⬆️
peppy/project.py 87.63% <ø> (+0.70%) ⬆️
peppy/sample.py 76.04% <ø> (+0.64%) ⬆️
peppy/utils.py 73.61% <ø> (+4.38%) ⬆️
tests/smoketests/conftest.py 100.00% <ø> (ø)
tests/smoketests/test_Project.py 99.16% <ø> (+0.16%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9392e6...c76c574. Read the comment docs.

@stolarczyk stolarczyk marked this pull request as ready for review June 8, 2021 13:12
@stolarczyk stolarczyk mentioned this pull request Jul 1, 2021
3 tasks
@nsheff
Copy link
Contributor

nsheff commented Nov 8, 2021

@stolarczyk Just need a quick comment on this -- right now the tests are failing because peppy can't import a PEP that's version 2.0.0 -- it says to use a different version:

E peppy.exceptions.InvalidConfigFileException: PEP version is invalid: 2.0.0. Please use version 2.1.0 or import an older version of Project from a submodule in this package, e.g. from peppy.pep200 import Project

Shouldn't the same object be used for 2.0.0 and 2.1.0 since they are backwards compatible? I just want to confirm that this is an error in the version checking, and not in the actual PEPs, since that doesn't make sense to me.

@stolarczyk
Copy link
Member Author

I don't remember exactly what behavior did we decide on. I agree that it would be great if one object could accommodate both versions.

In 2.1.0 the Project object can be created based on sample table, no project config is needed.
Maybe there were some related issues that prevented me from implementing it this way.

@nsheff
Copy link
Contributor

nsheff commented Nov 9, 2021

a separate object for 1.0 vs 2.0 makes sense to me, but not for 2.1 vs 2.0.

You had this working with all the 2.1 updates, before we decided to increment the spec to 2.1. Are you saying to update the spec you moved that all into a separate object so 2.0 and 2.1 are separate? I don't quite understand.

I think the issue is more likely just that you didn't update the tests -- but look, tests passed in your previous commit (the one prior to the last commit on this branch, which is what broke the tests.

@nsheff
Copy link
Contributor

nsheff commented Nov 16, 2021

@stolarczyk I'm still not sure what you were intending here so any guidance would be appreciated.

@nsheff
Copy link
Contributor

nsheff commented Nov 16, 2021

This commit solves most of the tests by not requiring the pep version to be 2.1.0. However, there are 2 tests that still fail because no pep version is included. Can you explain what you're trying to do here? In your tests, you load a version 1 PEP, I guess with a version 2 object, and then it complains that it lacks the version key. But version 1 PEPs aren't supposed to have the version key. So I guess this is just another error with the tests?

@nsheff
Copy link
Contributor

nsheff commented Nov 16, 2021

There are now 2 duplicates of the project code: in pep200/project.py is, I guess the "old" version -- which handles PEP version 1.0. I am just confused at the naming here; why did you make pep200 the one that handles PEP 1.0? Is it just a naming error and that should be pep100?

The main project in peppy/project.py handles either 2.0 or 2.1 PEPs.

@stolarczyk
Copy link
Member Author

Looks good to me. Make sure it's tested with looper before release.

@nsheff
Copy link
Contributor

nsheff commented Feb 9, 2022

@nleroy917 Are you satisfied that this is working in your use case?

@nleroy917
Copy link
Member

Yes it looks good to me

@nsheff
Copy link
Contributor

nsheff commented Feb 28, 2022

getting ready to release...

@nsheff nsheff merged commit a326a6c into master May 3, 2022
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 this pull request may close these issues.

3 participants