Skip to content

v0.16.0#175

Merged
UnHumbleBen merged 20 commits intomainfrom
dev
May 23, 2021
Merged

v0.16.0#175
UnHumbleBen merged 20 commits intomainfrom
dev

Conversation

@UnHumbleBen
Copy link
Copy Markdown
Collaborator

No description provided.

dave-doty and others added 19 commits January 15, 2021 15:41
Remove debug print hello in to_idt_bulk_input_format
* Fixes #163; Remove Helix.pitch and Helix.yaw

Completed:
* Remove Helix.pitch and Helix.yaw
* Update Design.from_json to be compatible with designs that specified
  pitch and yaw in individual Helix (except when multiple helices in the
  same group have different pitch/yaw values, in which case,
  IllegalDesignError is raised)
* Updated examples that used pitch and roll
* Updated unit tests that used pitch/roll
* Added new unit test to test new Design.from_json
* Add Design method for looking up pitch/yaw/roll for helices
Using re.sub (re is already imported), along with a replacement
function that pulls from the replacement map, avoids repeated string
replacements within Python and significantly speeds up encoding for
large files.  Note that I think this will raise a KeyError if the user has
something with a name fitting "@@(\d+)@@" where the number is larger
than the largest unique_id, and will corrupt the file if it is not.
However, this is similar to the current situation.
Encoder indent suppression: str.replace → re.sub (fixes #170)
…inx-warning

Fix Sphinx warning on idt_dna_sequence
@UnHumbleBen UnHumbleBen requested a review from dave-doty as a code owner May 13, 2021 06:56
@UnHumbleBen
Copy link
Copy Markdown
Collaborator Author

UnHumbleBen commented May 13, 2021

doc test failed even though docs compiled just fine locally.

I suspect it is because the action we are using depends on an earlier version of Sphinx (2.4.4)

When I run

pip install -Iv sphinx==2.4.4

to installed that version, and run make html I get build error:

Running Sphinx v2.4.4

Extension error:
Event 'html-collect-pages' already present
Makefile:20: recipe for target 'html' failed
make: *** [html] Error 2

The action does not seem reliable as it has been more than a year since last update on the repo and it does not have a versioning to ensure stability going into the future. Probably time to revisit the doc check github action so that it doesn't rely on their action.

In any case, I'm sure the action is failing due to a bug in their github action and not our docs

Copy link
Copy Markdown
Member

@dave-doty dave-doty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not merge all of these changes yet. In particular, there are breaking changes with regard to removing pitch and yaw from Helix, and I want to make sure we are handling that properly both here and in the Dart code before merging into main.

Is there a way to include only the commit(s) involving changing the name of the main branch?

Also, it would be good to find a new way to have a GitHub Action that tests if the docs compile properly.

@UnHumbleBen
Copy link
Copy Markdown
Collaborator Author

UnHumbleBen commented May 13, 2021

Is there a way to include only the commit(s) involving changing the name of the main branch?

A hotfix branch could be made that merges directly to main containing just the change to the workflow. Alternatively, we can just wait since the only workflow that was affected is release.yml (building and publishing to PyPI) and this workflow doesn't run until we merge to main anyways, so I think it is fine to hold off making this change directly to main and instead wait to merge this with the rest of dev.

Also, it would be good to find a new way to have a GitHub Action that tests if the docs compile properly.

Agreed, I will make an issue on this

@UnHumbleBen UnHumbleBen mentioned this pull request May 13, 2021
@UnHumbleBen UnHumbleBen changed the title Dev v0.16.0 May 17, 2021
@UnHumbleBen
Copy link
Copy Markdown
Collaborator Author

scadnano PR v0.16.0 ready!

@dave-doty
Copy link
Copy Markdown
Member

Can we try to fix the "Docs Check" action as part of this PR as well? I'd like to get that back to a state where it doesn't have a false alarm. (I haven't compiled the docs locally recently, so I don't even know whether the current alarm is false, but the check is not passing.)

@UnHumbleBen
Copy link
Copy Markdown
Collaborator Author

Can we try to fix the "Docs Check" action as part of this PR as well? I'd like to get that back to a state where it doesn't have a false alarm. (I haven't compiled the docs locally recently, so I don't even know whether the current alarm is false, but the check is not passing.)

Ok, I'll look into it

@UnHumbleBen
Copy link
Copy Markdown
Collaborator Author

UnHumbleBen commented May 17, 2021

@dave-doty While working on docs check workflow, I found that you can subscribe to notifications on the ReadTheDocs project site so that you get notified via email if a build fails.

I'm still going to try to add a workflow to do the doc check, but thought this is good to know

Merged from PR #178

* Update docs-check.yml

* Fix typo: "docs" -> "doc"

* Move "make html" to same shell as "cd" command

* Use sphinx-build command with -W flag

This makes it so that warnings are treated as errors (consistent with Read the Docs)
@UnHumbleBen UnHumbleBen requested a review from dave-doty May 23, 2021 19:12
Copy link
Copy Markdown
Member

@dave-doty dave-doty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add to the release notes that there is a breaking change?

It would also be good to document this change: fixes #168: ensure grid fields are of type Grid, not str or None, since it might break existing code that was manually passing in strings for the grid parameter.

@UnHumbleBen UnHumbleBen merged commit aa7a624 into main May 23, 2021
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