Ensure all table names originate from Lhm::TableName #49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 27, 2018 12:54
Because the table name behavior was distributed all over the system, it was not under test. This commit consolidates this behavior and puts it under test.
As you can see, the logic for describing table names is all over the place. Trying to change the interfaces for these classes is beyond the scope of the PR, as these interfaces are sadly coupled to the test suite. However, we can at least have these classes delegate the logic for these table names to something is well tested and centralized. This eliminates the tests currently running on migration_spec.
bbuchalter
requested review from
jordanwheeler,
insom,
berlincount and
ajmaidak
|
This solves our primary problem. It doesn't change the length of the timestamp, because #50, but at this point, we shouldn't care about that. |
jordanwheeler
approved these changes
Aug 27, 2018
berlincount
reviewed
Aug 28, 2018
| end | ||
|
|
||
| def archive_name | ||
| "lhma_#{ startstamp }_#{ @origin.name }"[0...64] |
|
|
||
| attr_reader :original | ||
|
|
||
| def archived |
There was a problem hiding this comment.
Not sure whether you end up with 65 instead of 64 bytes in all of these cases?
There was a problem hiding this comment.
but if that's the case, how does the test pass? I'm confused.
There was a problem hiding this comment.
@berlincount With three dots, ranges in Ruby are not inclusive:
irb(main):013:0> 'asdasdasdasdasdasdasdasdasdasdasdasdasdasdadasdasdasdasdasdasdasdasdasdadasd'[0...64].length
=> 64
With two dots they are. It's not my favourite syntax 😁
There was a problem hiding this comment.
what. the. beep.
Ruby magic strikes again.
insom
approved these changes
Aug 28, 2018
berlincount
approved these changes
Aug 28, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As you can see, the logic for describing table names is all over the place.
Trying to change the interfaces for these classes is beyond the scope of
the PR, as these interfaces are sadly coupled to the test suite.
However, we can at least have these classes delegate the logic for these
table names to something is well tested and centralized. This eliminates
the tests currently running on migration_spec.
Closes #47