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

PR: Add support for compiling directories of QtSass #23

Merged
merged 4 commits into from
May 21, 2018

Conversation

danbradham
Copy link
Collaborator

@danbradham danbradham commented May 16, 2018

fixes #18

  • Added complex example including imports from subdirectories and partial files
  • Simplified watchdog event handler
    • Now we just recompile anytime the source directory changes
    • This is far more reliable and accounts for imports
    • Removes weird logic to work around MacOS fsEvents
  • Aligned api compile* names with libsass
  • Decided not to modify cli arguments as discussed in CLI - build/watch directory #18 since input works fine for files or directories

I think the compile methods should be aligned closer to libsass in that sass.compile handles strings, files, and directories and delegates to sass.compile_filename and sass.compile_dirname. This pull request lays the foundation to achieve that by renaming compile_and_save to compile_filename and adding the compile_dirname method.

- Added complex example including imports from subdirectories
- Simplified watchdog event handler
  - Now we just recompile anytime the source directory changes
  - This is far more reliable and accounts for imports
- Aligned api compile* names with libsass
@danbradham danbradham added this to the v0.1.0 milestone May 16, 2018
@danbradham danbradham requested a review from goanpeca May 16, 2018 16:56
@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #23 into master will increase coverage by 52.15%.
The diff coverage is 78.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #23       +/-   ##
===========================================
+ Coverage   32.79%   84.95%   +52.15%     
===========================================
  Files           8        8               
  Lines         186      206       +20     
===========================================
+ Hits           61      175      +114     
+ Misses        125       31       -94
Impacted Files Coverage Δ
qtsass/conformers.py 95.55% <100%> (+17.77%) ⬆️
qtsass/__main__.py 100% <100%> (+100%) ⬆️
qtsass/__init__.py 100% <100%> (ø) ⬆️
qtsass/events.py 50% <33.33%> (+50%) ⬆️
qtsass/api.py 74% <66.66%> (+29.55%) ⬆️
qtsass/cli.py 77.77% <91.3%> (+77.77%) ⬆️
qtsass/importers.py 96.66% <93.75%> (+74.92%) ⬆️
... and 3 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 7e9855f...c206149. Read the comment docs.

@goanpeca
Copy link
Member

I think the compile methods should be aligned closer to libsass in that sass.compile handles strings, files, and directories

I understand the feeling but I would prefer to have separate methods for this and not a universal method that does all. or having separate argument?

@danbradham
Copy link
Collaborator Author

Yeah, I'm not very fond of universal methods or flag arguments that modify the behavior of a function drastically. I created issue #24 to track this discussion. I think any change in that regards to the api behavior should occur in a separate pull request.

@danbradham
Copy link
Collaborator Author

Added some cli tests including compiling and watching both examples/dummy.scss and examples/complex.

@goanpeca
Copy link
Member

Hi @danbradham this is looking great :-), vert thorough. You happy with the current state, or is there something else you want to work on?

@danbradham
Copy link
Collaborator Author

Thanks, yeah it's looking good to me.

@goanpeca goanpeca merged commit 54cf917 into spyder-ide:master May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI - build/watch directory
3 participants