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

Started docstring and test coverage on Tag class #329

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

sveetch
Copy link

@sveetch sveetch commented Aug 8, 2022

Pull Request Check List

re: #168

  • Added tests for changed code.
  • Updated documentation for changed code.

This is and attempt to improve quality. I've started it from beast_mode as a draft. I made some tests to understand the project code and it leaded to documentation and reference tests so i would be able to know how to contribute to the new parser refactoring.

@christopherpickering
Copy link
Contributor

@sveetch nice, thanks! What purpose does the makefile serve? Faster for you to get the project installed? I would personally add it to the gitignore.

The fixtures are looking good too. I'm still now completely satisfied with the html statements split into a list of strings, with the big print statement following, but we can revisit it.

Are you continuing to work this pr, or can I start merging it in?

Thanks!

@sveetch
Copy link
Author

sveetch commented Aug 16, 2022

@christopherpickering yeah the Makefile is an easy entrypoint to install and performing some tasks around project. I tend to think this is commonly easier for people to quickly manipulate a project, instead of learning poetry or to learn other commands that poetry can not manage.

Whatever, i will remove it if you are not confident with it.

About the stage of this PR, it is not totally finished, i had some few method to cover for Tag, so don't merge it now, i will see when i can finish it this week, i will get back to you on discord to talk about all of this soon.

@sveetch
Copy link
Author

sveetch commented Aug 21, 2022

@christopherpickering Ok i've finished the docstring and test coverage on Tag class.

Note than:

  • The tests i've done is in "reference_tests" directory since they are totally different of your ones from tests/ dir. In the end it should be merged in test/ dir but for now they should live appart;
  • There is many of "TODO" line which are notices about some incongruities and weird behaviors i found during coverage;
  • poetry.lock and pyproject.toml have been automatically updated by poetry because of the development environment enabled, i don't know enough poetry to avoid this, whatever you can omit these file changes;

Edit: Oh yeah also, i've squash my many commits to one (for history consistency) and push forced, so you better be to remove the branch locally, fetch it and create it again.

src/djlint/formatter/utils/tag.py Show resolved Hide resolved
Indentation sensitivity is based on element CSS flow. If element is HTML and
have a 'pre' alike flow it is considered as sensitive.

TODO: It seems '__tag_is_pre' have already been used in 'self.is_pre' from
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

src/djlint/formatter/utils/tag.py Show resolved Hide resolved
Whatever script status is, if ``Tag._is_html`` is ``False`` this will always
return ``False``.

TODO: <template> element should probably be assumed as a script element.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prob keep template separate. The goal of this prop is to allow the contents to be formatted w/ a js formatter, and prevent djlint from touching the contents.

I think the contents of a template tag we are generally safe to format.

@christopherpickering christopherpickering marked this pull request as ready for review October 10, 2022 08:15
@christopherpickering christopherpickering merged commit b201ca1 into djlint:beast_mode Oct 10, 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.

2 participants