Hotfix/empty export warning #165

jimfoltz · 2017-01-07T22:33:17Z

Fixes #159

The STL Exporter would happily export a .stl file with no facets if the "Export selection only" option was checked but nothing in the model was actually selected.

thomthom · 2017-01-12T21:14:07Z

src/sketchup-stl/exporter.rb

+            @write_face = method(:write_face_ascii)
+         end
+         scale = scale_factor(options['export_units'])
+         write_header(file, model_name(), options['stl_format'])


Style guide says to not use parentheses for method calls with no arguments; model_name()

I know - I just never liked the way a name in Ruby could either be a variable or method. WIll comply.

I know what you mean. Though it appear all Ruby style guides disagree, so do the IDE linters. Better to just follow the main stream.

thomthom · 2017-01-12T21:14:52Z

src/sketchup-stl/exporter.rb

@@ -230,6 +225,27 @@ def self.get_vertex_order(positions, face_normal)
        order
      end

+      # Return model.active_entites, selection, or nil


Should we use YARD style comments for this?

Maybe? That would be the only YARD style comment in the entire source.

It was just a question though. You can leave this as-is if you want.

thomthom · 2017-01-12T21:15:33Z

src/sketchup-stl/exporter.rb

+        else
+          export_ents = Sketchup.active_model.active_entities
+        end
+        return export_ents


Style guide says to not use return unless returning early - instead rely on the implicit return.

thomthom · 2017-01-12T21:17:29Z

src/sketchup-stl/exporter.rb

+            msg = "SketchUp STL Exporter:\n\n"
+            msg << "You have chosen \"Export only current selection\", but nothing is selected."
+            msg << "\n\nWould you like to export the entire model?"
+            if UI.messagebox(msg, MB_YESNO) == IDYES


So this warning only appear if the user set the export options to Selection with no selection made?

Yes. How much hand-holding is the right amount? Again, I based this on the Pro exporters opting for consistency.

I am more concerned about keeping message boxes to a minimum than the hand-holding. This is fine as it's an edge-case that the user should not run into often.

thomthom · 2017-01-12T21:17:56Z

src/sketchup-stl/exporter.rb

@@ -261,7 +277,7 @@ def self.do_options

        # Row 1 Export Selected
        chk_selection = SKUI::Checkbox.new(
-          'Export selected geometry only.',
+          'Export only current selection',


Is 'current' redundant?

I copied the text from the other Pro exporters - I know that doesn't necessarily make it right but it is consistent.

Gotcha, then lets keep in consistent.

thomthom · 2017-01-12T21:18:32Z

src/sketchup-stl/exporter.rb

          control.window.close
+          export_entities = get_export_entities()


Style nit: not empty parentheses with empty method calls.

thomthom · 2017-01-12T21:30:09Z

src/sketchup-stl/exporter.rb

+             path = select_export_file()
+             begin
+                export(path, export_entities, OPTIONS) unless path.nil?
+             rescue => exc


Style guide says to prefer full names over abbreviations. exc would be better expanded to exception if that was what you meant. However, default catch is StandardError in this case.

thomthom · 2017-01-12T21:31:57Z

src/sketchup-stl/exporter.rb

+      # show the export dialog.
+      def self.main
+        if Sketchup.active_model.active_entities.length == 0
+          msg = "SketchUp STL Exporter:\n\n" +


Style guide says to avoid + to concat strings. I think << is preferred over +, but I also think you can do something like this in Ruby:

str = "Hello"\ "World"

https://github.com/bbatsov/ruby-style-guide#method-invocation-parens

https://github.com/bbatsov/ruby-style-guide#no-explicit-return

thomthom · 2017-01-16T21:33:08Z

src/sketchup-stl.rb

@@ -52,7 +52,7 @@ def self.translate(string)
      'This is an open source project sponsored by the SketchUp team. More ' <<
      'info and updates at https://github.com/SketchUp/sketchup-stl'
    )
-    extension.version = '2.1.6'


Why was this bumped from 2.1.6 to 2.1.8? What about 2.1.7?

Oh, the normal fix was going to be 2.1.7 but maybe I didn't push the tag.

Ah - I see. I don't think we published that. So on EW it's still 2.1.6. Can you make a new commit with version set to 2.1.7? Then I'm squash-merge this and get it uploaded.

Well, I've tagged the commit in my repo but not sure what to do now. Just need to tag commit #fefa9098be9600efb5ac7e49ef52777cc973fe76 with v2.1.7

Tags in your repo doesn't follow the Pull Request - I think. The tag is something I have to set in order for it to apply to this repo.

Doesn't matter for me. We usually end up with a couple of commits after a PR is done anyway - in which case we squash everything.

Small changes can help follow the thought process better - that's the main reason I might prefer to keep them.

You mean in the master branch?

You mean in the master branch?

No, but in the dvelopment branches.

Yea, that's perfectly fine and I think it's a common git workflow.

jimfoltz added 10 commits January 7, 2017 04:56

remove menu validation proc

ffef4e9

add main() entry point which checks for empty model

ca9a0d3

call new main() method from menu

4f1e098

rename selection only option text to match other Pro exporters

9ad5e7a

added get_export_entities method to check entities before exporting

5a7e6bb

done with the export dialog here

bdd93f8

comments and formatting

c878bc6

bump version

1df7f1c

bump version

de79db0

edit version

0247a5a

thomthom requested changes Jan 12, 2017

View reviewed changes

jimfoltz added 6 commits January 12, 2017 17:47

Omit parenthesis for method calls with no arguments

0115243

https://github.com/bbatsov/ruby-style-guide#method-invocation-parens

Avoid return where not required

98c8543

https://github.com/bbatsov/ruby-style-guide#no-explicit-return

style: avoid unnecessary parentheses

6ce91a7

style: prefer full variable names over abbreviations

c1f61cb

style: prefer << over + for string concatenation

68007ab

style: do not use parentheses on method calls with no arguments

5004dc1

thomthom approved these changes Jan 13, 2017

View reviewed changes

thomthom reviewed Jan 16, 2017

View reviewed changes

thomthom merged commit ef3e6e2 into SketchUp:master Jan 25, 2017

jimfoltz deleted the hotfix/empty_export_warning branch January 25, 2017 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hotfix/empty export warning #165

Hotfix/empty export warning #165

jimfoltz commented Jan 7, 2017

thomthom Jan 12, 2017

jimfoltz Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

jimfoltz Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

jimfoltz Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

jimfoltz Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 12, 2017

thomthom Jan 16, 2017

jimfoltz Jan 16, 2017

thomthom Jan 16, 2017

jimfoltz Jan 16, 2017

thomthom Jan 16, 2017 •

edited

Loading

thomthom Jan 16, 2017

jimfoltz Jan 16, 2017

thomthom Jan 16, 2017

jimfoltz Jan 16, 2017

thomthom Jan 16, 2017

Hotfix/empty export warning #165

Hotfix/empty export warning #165

Conversation

jimfoltz commented Jan 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomthom Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomthom Jan 16, 2017 •

edited

Loading