move err msg strings to frozen constants #1

iirving · 2023-09-26T17:33:00Z

moving Error message string into constant string that are frozen.

at minimum you should freeze strings in place

Since there isn't any reuse making a constant is low value, but giving them a constant name is the beginning of giving them a key for a external file for i13n values

smiller · 2023-09-28T01:25:56Z

Hmm. I would probably do this with

# frozen_string_literal: true

at the top of the file instead of multiple .freeze calls.

https://docs.ruby-lang.org/en/3.2/syntax/comments_rdoc.html#label-frozen_string_literal+Directive

smiller · 2023-09-28T01:36:09Z

Also, looking for and not finding the test run results, this shows that the github action file should be updated from

on: [push]

to

on: [push, pull_request]

smiller · 2023-09-28T02:00:24Z

Also I think these constant names make more sense to me:

ERR_INPUT_REQ =>  ERROR_INPUT_OPTION_MISSING
ERR_OUTPUT_REQ => ERROR_OUTPUT_OPTION_MISSING
ERR_INPUT_MISSING => ERROR_INPUT_FILE_NOT_FOUND
ERR_OUTPUT_MISSING => ERROR_OUTPUT_FILE_FOUND

iirving · 2023-09-28T02:22:18Z

as there are no other strings in file # frozen_string_literal: true makes sense. I'm more used to individually freezing strings. 👍
I'll update inline your more complete and explicit (aka wordy aka meaningful) constant names. 👍
I'll revise the PR and we can test github action. You're welcome! :)

iirving · 2023-09-28T02:37:46Z

@smiller changes made, action is requiring your approval before running 🤞

bow down before my power (🔋 ) and abuse ( 😈 ) of emjoi's!
🙈 🙉 🙊 !!

smiller · 2023-09-28T12:01:24Z

Hmm. The approved action ran and failed. Not for cause – from the run-tests step, the tests passed and the coverage was maintained, but the SimpleCov Report step failed with the complaint Error: Resource not accessible by integration, though the RSpec Report and the Upload coverage results steps were able to access the files they needed. I re-ran in case it was an intermittent failure and it failed the same way.

Possibly there are different permissions for the GITHUB_TOKEN used in the pull request case? I'll keep digging, and the github action file might need to change again accordingly. I'd like this to run green before I pull it in, but there are no further changes required to the code. Just possibly the action.

This has been a useful exercise, thank you!

smiller · 2023-09-28T12:57:52Z

Hmm. Went into settings and temporarily gave github actions default “read and write permissions” instead of the basic default of “Read repository contents and packages permissions” and re-ran and it's still failing. (Which, it shouldn't need to write to access the coverage file and generate a report from it, so, that's probably reasonable.)

smiller · 2023-09-28T13:03:19Z

And a push to a local branch with the same changes as the PR passes with no problem – https://github.com/smiller/format-dual-language/actions/runs/6339261819 – so it's definitely something to do with the PR case.

smiller · 2023-09-28T13:17:48Z

But the same push to a local branch turned into a PR fails in the same place on SimpleCov Report: https://github.com/smiller/format-dual-language/actions/runs/6339437587/job/17218559180

smiller · 2023-09-28T02:08:50Z

lib/processor.rb

+  ERR_INPUT_REQ = "-i FILE option for input file required".freeze
+  ERR_OUTPUT_REQ = "-o FILE option for output file required".freeze
+  ERR_INPUT_MISSING = "input file must exist".freeze
+  ERR_OUTPUT_MISSING = "output file must not exist".freeze


Suggested change

ERR_INPUT_REQ = "-i FILE option for input file required".freeze

ERR_OUTPUT_REQ = "-o FILE option for output file required".freeze

ERR_INPUT_MISSING = "input file must exist".freeze

ERR_OUTPUT_MISSING = "output file must not exist".freeze

ERROR_INPUT_OPTION_MISSING = "-i FILE option for input file required"

ERROR_OUTPUT_OPTION_MISSING = "-o FILE option for output file required"

ERROR_INPUT_FILE_NOT_FOUND = "input file must exist"

ERROR_OUTPUT_FILE_FOUND = "output file must not exist"

Implicit in this suggestion is of course also adding

# frozen_string_literal: true

to the top of the file.

smiller · 2023-09-28T02:10:11Z

lib/processor.rb

@@ -13,17 +18,17 @@ def validate
    @errors = []

    if !@options.has_key?(:in)
-      @errors << "-i FILE option for input file required"
+      @errors << ERR_INPUT_REQ


Suggested change

@errors << ERR_INPUT_REQ

@errors << ERROR_INPUT_OPTION_MISSING

smiller · 2023-09-28T02:10:51Z

lib/processor.rb

    end
    if !@options.has_key?(:out)
-      @errors << "-o FILE option for output file required"
+      @errors << ERR_OUTPUT_REQ


Suggested change

@errors << ERR_OUTPUT_REQ

@errors << ERROR_OUTPUT_OPTION_MISSING

smiller · 2023-09-28T02:10:57Z

lib/processor.rb

    end

    if @options.include?(:in) && !File.file?(@options[:in])
-      @errors << "input file must exist"
+      @errors << ERR_INPUT_MISSING


Suggested change

@errors << ERR_INPUT_MISSING

@errors << ERROR_INPUT_FILE_NOT_FOUND

smiller · 2023-09-28T02:11:15Z

lib/processor.rb

    end
    if @options.include?(:out) && File.file?(@options[:out])
-      @errors << "output file must not exist"
+      @errors << ERR_OUTPUT_MISSING


Suggested change

@errors << ERR_OUTPUT_MISSING

@errors << ERROR_OUTPUT_FILE_FOUND

move err msg strings to frozen constants

34c4477

iirving marked this pull request as draft September 27, 2023 21:49

iirving marked this pull request as ready for review September 27, 2023 21:50

iirving added 2 commits September 27, 2023 22:27

use file wide frozen_string_literal Directive

e8b4fd0

update constant error msg names

e501d0e

smiller approved these changes Sep 28, 2023

View reviewed changes

smiller merged commit 12b4c68 into smiller:main Sep 28, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move err msg strings to frozen constants #1

move err msg strings to frozen constants #1

iirving commented Sep 26, 2023 •

edited

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

iirving commented Sep 28, 2023

iirving commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller Sep 28, 2023

smiller Sep 28, 2023

smiller Sep 28, 2023

smiller Sep 28, 2023

smiller Sep 28, 2023

smiller Sep 28, 2023

	@errors << ERR_INPUT_REQ
	@errors << ERROR_INPUT_OPTION_MISSING

	@errors << ERR_OUTPUT_REQ
	@errors << ERROR_OUTPUT_OPTION_MISSING

	@errors << ERR_INPUT_MISSING
	@errors << ERROR_INPUT_FILE_NOT_FOUND

	@errors << ERR_OUTPUT_MISSING
	@errors << ERROR_OUTPUT_FILE_FOUND

move err msg strings to frozen constants #1

move err msg strings to frozen constants #1

Conversation

iirving commented Sep 26, 2023 • edited

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

iirving commented Sep 28, 2023

iirving commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller commented Sep 28, 2023

smiller Sep 28, 2023

Choose a reason for hiding this comment

smiller Sep 28, 2023

Choose a reason for hiding this comment

smiller Sep 28, 2023

Choose a reason for hiding this comment

smiller Sep 28, 2023

Choose a reason for hiding this comment

smiller Sep 28, 2023

Choose a reason for hiding this comment

smiller Sep 28, 2023

Choose a reason for hiding this comment

iirving commented Sep 26, 2023 •

edited