Skip to content

export/excel: export special fields#282

Merged
stanislaw merged 1 commit intostrictdoc-project:masterfrom
stumpyfr:feature/excel_fields
Jan 3, 2021
Merged

export/excel: export special fields#282
stanislaw merged 1 commit intostrictdoc-project:masterfrom
stumpyfr:feature/excel_fields

Conversation

@stumpyfr
Copy link
Copy Markdown

@stumpyfr stumpyfr commented Jan 3, 2021

  • poetry run strictdoc export ~/Desktop/apex3-4.sdoc --formats=html,excel --fields=uid,statement,owner,priority,parent is now working.
  • unknown fields will create an empty column
  • ran black on the modified files.

@stumpyfr
Copy link
Copy Markdown
Author

stumpyfr commented Jan 3, 2021

if you approve this idea, I can check to fix these unit tests.

@stanislaw
Copy link
Copy Markdown
Collaborator

stanislaw commented Jan 3, 2021

All looks good. Please proceed with writing one or more integration tests.

Check out the invoke --list command.

You mostly need invoke test to run all checks.

I think I have only written an integration test, not unit tests for Excel export. If you have a good idea of how to unit test this code as well, please feel free to add them as well. (invoke test-unit).

For integration tests:

You can use invoke test-integration --focus 01_basic_excel_export to run just one LIT test.

The idea there is that the expected folder contains the file you expect to be generated. So for the new export feature with special fields, you need to run the export, copy the file to the expected folder. Otherwise your test should follow the same template as strictdoc/tests/integration/commands/export/excel/01_basic_excel_export.

If you run everything using Poetry, then you need poetry run invoke test.

@stumpyfr
Copy link
Copy Markdown
Author

stumpyfr commented Jan 3, 2021

Done.
for the integration/E2E tests, maybe worth a look to https://behave.readthedocs.io/en/stable/
Pretty fan of this package to easily describe "a la BDD" the features and test them.

@stanislaw
Copy link
Copy Markdown
Collaborator

I have checked out your branch and all works and looks good.

Could you please git rebase -i your branch on top of the latest master and also squash your commits into just one?

I am generally following the following convention for the commit titles:

feature: optional header: description

In this case, something like export/excel: export special fields would work.

@stumpyfr stumpyfr force-pushed the feature/excel_fields branch from 743312f to f933f43 Compare January 3, 2021 16:50
@stumpyfr stumpyfr force-pushed the feature/excel_fields branch from f933f43 to af5f095 Compare January 3, 2021 16:52
@stanislaw
Copy link
Copy Markdown
Collaborator

@stumpyfr great work! Thanks for rebasing. One itest that could be written later would be to test exporting all possible fields to Excel. But that's another story.

@stanislaw stanislaw merged commit 6575f78 into strictdoc-project:master Jan 3, 2021
@stanislaw stanislaw changed the title fields argument for the excel export export/excel: export special fields Jan 3, 2021
@stanislaw
Copy link
Copy Markdown
Collaborator

Another heads up: GitHub still does not show you as a contributor. It seems to be the behavior of the GitHub merging strategy when it is set to Rebase when merging the PRs.

I will use merge strategy from now on. Then your commits are yours and merge commits would be mine. (now it is single commit that has us both as co-authors).

Again, in case if this matters to you a lot, I didn't know about this behavior of GitHub either and from now on I will use the merge strategy instead of rebase.

@stumpyfr
Copy link
Copy Markdown
Author

stumpyfr commented Jan 3, 2021

no problem :)

@stumpyfr stumpyfr deleted the feature/excel_fields branch January 3, 2021 17:13
@stanislaw
Copy link
Copy Markdown
Collaborator

Done.
for the integration/E2E tests, maybe worth a look to https://behave.readthedocs.io/en/stable/
Pretty fan of this package to easily describe "a la BDD" the features and test them.

I have worked with the BDD style tests myself but in the context of testing web applications.

Applying it to the command line applications is something that I have not considered before. Also, I didn't know about this specific tool. I am going to check it out. Thanks for the hint.

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.

2 participants